search

NREL / OpenOA

This library provides a framework for assessing wind plant performance using operational assessment (OA) methodologies that consume time series data from wind plants. The goal of the project is to provide an open source implementation of common data structures, analysis methods, and utility functions relevant to wind plant OA.
https://openoa.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
194 stars 63 forks source link

Bug Fix: Update Pandas Offsets Usage #271

Closed RHammond2 closed 9 months ago

RHammond2 commented 9 months ago

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:

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

codecov-commenter commented 9 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

RHammond2 commented 9 months ago

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

jordanperr commented 9 months ago

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 commented 9 months ago

@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.