Skip to content

Bug Fix: Update Pandas Offsets Usage#271

Merged
RHammond2 merged 26 commits intoNatLabRockies:developfrom
RHammond2:fix/pandas-offsets
Feb 9, 2024
Merged

Bug Fix: Update Pandas Offsets Usage#271
RHammond2 merged 26 commits intoNatLabRockies:developfrom
RHammond2:fix/pandas-offsets

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

❌ Patch coverage is 98.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.28%. Comparing base (dfc49cd) to head (3035529).
⚠️ Report is 29 commits behind head on develop.

Files with missing lines 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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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 NatLabRockies:develop Feb 9, 2024
@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