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

Bug Fix: Update Pandas Offsets Usage #271

Merged
merged 26 commits into from
Feb 9, 2024

Conversation

RHammond2
Copy link
Collaborator

This PR addresses the underlying issue causing all CI tests to fail in current PRs. The issue stems from a change in expected datetime offsets used in Pandas. From #269:

  • N -> ns
  • S -> s
  • T -> min
  • H -> h
  • U -> us
  • L -> ms

All underlying code that relied on the previously acceptable codes have been replaced. This will inevitably lead to a minor version bump due to some changes in usage.

Closes #269

@RHammond2 RHammond2 added bug maintenance Issues related to code maintainence. E.g., upgrading versions of dependencies, fixing the CI pipelie labels Feb 6, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (dfc49cd) 75.31% compared to head (3035529) 77.28%.
Report is 2 commits behind head on develop.

Files Patch % Lines
openoa/analysis/aep.py 93.75% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #271      +/-   ##
===========================================
+ Coverage    75.31%   77.28%   +1.96%     
===========================================
  Files           29       29              
  Lines         3760     3693      -67     
===========================================
+ Hits          2832     2854      +22     
+ Misses         928      839      -89     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@ejsimley ejsimley left a comment

Choose a reason for hiding this comment

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

Thanks for finding this change in the datetime offsets! This all looks good to me.

Just wanted to note that we made some updates to the 07_static_yaw_misalignment.ipynb notebook in the main branch that aren't in develop yet. Since the example notebooks are changed in this PR too, we should make sure all relevant updates to the notebook are kept when we release the next version.

@RHammond2 RHammond2 linked an issue Feb 7, 2024 that may be closed by this pull request
@RHammond2
Copy link
Collaborator Author

Thanks for pointing that out, @ejsimley, I'll merge main back into develop, and update this PR again.

@jordanperr
Copy link
Collaborator

Upgrading to new-style pandas time offsets will break the usage of old-style pandas time offsets that are passed through in OpenOA's API, or used outside of the API in user's code. For example, the "freq" argument in utils.qa.daylight_savings_plots, the "offset" argument in utils.timeseries.offset_to_seconds, and possibly the "time_resolution" argument in MonteCarloAEP.

To remain consistent with our semantic versioning, we should provide the users a fall-back to the old-style offsets if needed. One way this could work is to add a note about this deprecation issue to the Readme, advising users to downgrade Pandas themselves, and then add custom support for the old-style frequencies in our API (could simply translate the old strings to the new ones).

Or, alternatively, just pin Pandas below the version where this breaks for the 3.x line, silence the warning, and put this PR in the 4.x release.

@RHammond2
Copy link
Collaborator Author

@jordanperr I was able to add in some support for the "old" style of offset strings without much extra lift. The catch is that it will output a deprecation warning pushing users to adopt the "new" style. I also added a test for the method that is applied to the MonteCarloAEP.time_resolution attribute and the frequency attribute for the metadata classes.

@RHammond2 RHammond2 merged commit c016aae into NREL:develop Feb 9, 2024
6 checks passed
@RHammond2 RHammond2 deleted the fix/pandas-offsets branch February 9, 2024 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug maintenance Issues related to code maintainence. E.g., upgrading versions of dependencies, fixing the CI pipelie
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Pandas Offset Strings
4 participants