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

return results instead of none when CODS soiling signal is small #400

Merged
merged 13 commits into from
Dec 1, 2023

Conversation

martin-springer
Copy link
Collaborator

@martin-springer martin-springer commented Nov 17, 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

@mdeceglie
Copy link
Collaborator

I re-ran the offending notebook and scrutinized the output to be sure no substantial changes occurred in the outputs. That plot does appear to change size, but I don't think it is a concern.

"'{}' was expected as a column, but not in result_df".format(x)


def test_soiling_cods_small_signal(cods_normalized_daily_small_soiling):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the CODS small signal test

@martin-springer
Copy link
Collaborator Author

I did update all packages in my conda environment and saved the notebook. Somehow, there's still a discrepancy in one cell. I could try re-creating a new environment the exact way nbval does here. On the other hand, we get quite a lot of warning and deprecation messages. Might be time to start resolving them.

@mdeceglie
Copy link
Collaborator

mdeceglie commented Nov 30, 2023

I think we can look past the notebook failure this time. I opened #401 today because of the message we're seeing in the notebook.

@mdeceglie
Copy link
Collaborator

Let's get the change log updated, then I think we can move forward with this. The version will be 2.2.0-beta.2

@martin-springer
Copy link
Collaborator Author

martin-springer commented Dec 1, 2023

Sounds good, I updated the changelog. Could you merge these changes into aggregated_filters_for_trial as well and create a tag for the next pvfleets run?

Copy link
Collaborator

@mdeceglie mdeceglie left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @martin-springer!

@mdeceglie mdeceglie merged commit 250e412 into development Dec 1, 2023
15 of 16 checks passed
@mdeceglie mdeceglie mentioned this pull request Oct 23, 2024
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.

2 participants