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

Fix error with new numpy version #46

Merged
merged 16 commits into from
Jul 31, 2024
Merged

Fix error with new numpy version #46

merged 16 commits into from
Jul 31, 2024

Conversation

BStoelzner
Copy link
Contributor

np.product doesn't work with the new numpy, so I changed it to np.prod

@BStoelzner
Copy link
Contributor Author

The tests seem to fail because of an incompatibility between numpy and pandas, see numpy/numpy#26710
A popular solution at the moment seems to be to just downgrade numpy or pandas.

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.59%. Comparing base (60d6359) to head (064be05).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   97.97%   99.59%   +1.61%     
==========================================
  Files           7        7              
  Lines         742      738       -4     
==========================================
+ Hits          727      735       +8     
+ Misses         15        3      -12     

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

Copy link
Contributor

@hangqianjun hangqianjun 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 to me, and thanks for adding the unit tests!

One thing I'm not sure about is the numpy version limit - I think a number of people argued against that as it might make co-updating rail and numpy harder in future versions. People also tend to use the newer versions of numpy, and reverting to older version in order to use rail may be slightly non-ideal.

Having said that, setting numpy<2.0.0 is an effective and short term solution, because the issue presented here seems to be unsolvable within rail, and needs actions from external packages.

Any thought from other reviewers?

pyproject.toml Outdated
@@ -17,7 +17,8 @@ classifiers = [
dynamic = ["version"]
dependencies = [
"deprecated",
"pz-rail-base",
"pz-rail-base>=1.0.3",
"numpy<2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to set the numpy version limit?

@BStoelzner
Copy link
Contributor Author

Another option would be to require pandas>=2.2.2, as this seems to be the first version that is compatible with numpy 2.0, see https://pandas.pydata.org/docs/whatsnew/v2.2.2.html#pandas-2-2-2-is-now-compatible-with-numpy-2-0

@BStoelzner
Copy link
Contributor Author

I just tested setting pandas>=2.2.2, but this leads to a conflict with photerr because this requires pandas<2.0.0. @jfcrenshaw, is there a possibility to make photerr compatible with pandas 2.0?

@ztq1996
Copy link
Contributor

ztq1996 commented Jul 30, 2024

Okay here is the thing:

photerr==1.1.0 requires pandas>1.4.3 <2.0.0
Now photerr==1.3.0 has loosen the requirement to pandas>1.4.3 only, but it only supports python>3.10, so python 3.9 still won't work.

@ztq1996
Copy link
Contributor

ztq1996 commented Jul 30, 2024

ok

@ztq1996 ztq1996 merged commit 89d3ca8 into main Jul 31, 2024
5 checks passed
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.

5 participants