-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add python 3.11 and 3.12 versions to testing matrix #428
Add python 3.11 and 3.12 versions to testing matrix #428
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## aggregated_filters_for_trials #428 +/- ##
=================================================================
+ Coverage 96.09% 96.10% +0.01%
=================================================================
Files 11 11
Lines 2200 2209 +9
=================================================================
+ Hits 2114 2123 +9
Misses 86 86 ☔ View full report in Codecov by Sentry. |
@mdeceglie - Regarding numpy > 2. We would need pandas 2.2.2 (https://pandas.pydata.org/docs/whatsnew/v2.2.2.html). However, the bug you found in pandas is still persistent in 2.2.2 (pandas-dev/pandas#55794). Not sure, how to proceed... |
…ions-to-testing-matrix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a few minor comments to address, then it should be ready to go. Thanks!
@@ -531,6 +536,11 @@ def _combine_losses(self, rollup_period='M'): | |||
The period on which to roll up losses and calculate availability. | |||
""" | |||
|
|||
# Allow pandas < 2.0 to use 'M' as an alias for MonthEnd | |||
# https://pandas.pydata.org/docs/whatsnew/v2.2.0.html#deprecate-aliases-m-q-y-etc-in-favour-of-me-qe-ye-etc-for-offsets | |||
if rollup_period == "ME": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update the docstrings for _combine_losses and run to explain the situation. Maybe something like "Default value "ME" triggers monthly rollup period. For other aliases, be sure they are compatible with your version of Pandas"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I've updated the docstrings.
@@ -521,4 +519,4 @@ def test_hour_angle_filter(): | |||
|
|||
# Check that the result is the correct boolean Series | |||
expected_result = pd.Series([False, False, True, True, True], index=index) | |||
pd.testing.assert_series_equal(result, expected_result) | |||
pd.testing.assert_series_equal(result, expected_result, check_names=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What caused the need for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the newest pvlib version 0.11.1 there was a bug fix for the hour_angle() function (https://pvlib-python.readthedocs.io/en/stable/whatsnew.html).
The series that's returned does have a new argument Name: equation_of_time
.
In order to make the test run for older pvlib versions in which the name attribute is not present, I decided to not add the name to the expected result series but rather don't check for series names.
setup.py
Outdated
"h5py >= 3.7.0", | ||
"plotly>=4.0.0", | ||
"xgboost >= 1.3.3", | ||
"pvlib >= 0.9.0, <0.12.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requirments_min specifies 0.11. We should change the minimum here too right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely right, I overlooked that. I've bumped it to 0.11 now.
__init__.py