Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RdTools 3.0 #390

Merged
merged 248 commits into from
Oct 29, 2024
Merged

RdTools 3.0 #390

merged 248 commits into from
Oct 29, 2024

Conversation

mdeceglie
Copy link
Collaborator

@mdeceglie mdeceglie commented Sep 12, 2023

  • Code changes are covered by tests
  • Code changes have been evaluated for compatibility/integration with TrendAnalysis
  • New functions added to __init__.py
  • API.rst is up to date, along with other sphinx docs pages
  • Example notebooks are rerun and differences in results scrutinized
  • Updated changelog

Copy link
Collaborator

@cdeline cdeline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending a few small changes, Notebooks are good, Documentation looks good, Pytests are passing and test coverage is an IMPRESSIVE 96%! LGTM.

setup.py Outdated
'sphinx_rtd_theme==0.5.2',
'ipython',
"doc": [
"sphinx==8.0.2",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sphinx 8.0.2 requires python >= 3.10 but we claim compatibility with python 3.9. I'm ok with keeping this as-is since running docs isn't a core functionality, but if we can bump this down to sphinx==7.4.7 it would retain 3.9 compliance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @cdeline! Looks like we made that change in #428, @martin-springer any input or concerns? Otherwise I'm happy to try it and see if everything works.

Copy link
Collaborator

@martin-springer martin-springer Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdeceglie - I don't think there should be any major issues with downgrading sphinx. The only thing that might happen is that some other doc requirements need to be downgraded as well.

.readthedocs.yml has used python 3.7 in the past, which caused the documentation to fail on the current branch with all the updated packages. Thus, I had to update the python version for the readthedocs workflow and went straight to python 3.12 as we decided this to be our new default.

docs/sphinx/source/changelog/pending.rst Outdated Show resolved Hide resolved
docs/sphinx/source/changelog/pending.rst Outdated Show resolved Hide resolved
docs/degradation_and_soiling_example.ipynb Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
RdTools Change Log
==================
.. include:: changelog/pending.rst
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename pending.rst to v3.0.0.rst for the release..

@mdeceglie
Copy link
Collaborator Author

A couple more changes based on our internal discussions. If this still looks good to everyone, I will update the changelog with 3.0beta, add a date and get this merged to development so we can polish of the soiling changes ahead of the full release.

@martin-springer
Copy link
Collaborator

A couple more changes based on our internal discussions. If this still looks good to everyone, I will update the changelog with 3.0beta, add a date and get this merged to development so we can polish of the soiling changes ahead of the full release.

Looks good to me!

@mdeceglie mdeceglie merged commit b3927da into development Oct 29, 2024
19 checks passed
@mdeceglie mdeceglie deleted the aggregated_filters_for_trials branch October 29, 2024 03:30
@mdeceglie mdeceglie restored the aggregated_filters_for_trials branch October 30, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants