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

pin numpy <2 #222

Closed
wants to merge 1 commit into from
Closed

pin numpy <2 #222

wants to merge 1 commit into from

Conversation

andrewgsavage
Copy link
Contributor

theres some depreciations in numpy 2, so they recommend pinning to <2 until modules support /test it.

Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.74%. Comparing base (3255816) to head (792999b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #222   +/-   ##
=======================================
  Coverage   95.74%   95.74%           
=======================================
  Files          12       12           
  Lines        1905     1905           
=======================================
  Hits         1824     1824           
  Misses         81       81           

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

@newville
Copy link
Member

@andrewgsavage Sounds like a wise idea to me. Any objections to merging this?

@andrewgsavage
Copy link
Contributor Author

merge when ready

@wshanks
Copy link
Collaborator

wshanks commented May 15, 2024

A quick ruff check --select=NPY flags no compatibility issues and running the tests with numpy==2.0.0rc2 produces only one test failure -- test_broadcast_funcs fails asserting that numpy.acos does not exist but it was added in numpy 2.0. It is probably better just to adjust that test.

@newville
Copy link
Member

@wshanks Thanks for that! That's really good news, actually.

I would still vote for merging this PR for 3.2.0, and expect to "officially" support numpy2.0 support in the next release.

I expect that many libraries will consider numpy 2.0.0 to be "beta", and wait until the first or second patch release before supporting it.

@andrewgsavage
Copy link
Contributor Author

andrewgsavage commented May 16, 2024 via email

@wshanks
Copy link
Collaborator

wshanks commented May 16, 2024

The current test failure only involves a problem with the test itself and not with the library code. I opened #225 to address it. I also opened #224 as a reminder to review the numpy code more generally.

I don't feel too strongly either way about pinning numpy here for a couple reasons (so merge if you prefer to):

  • In all my projects, I have only listed uncertainties and numpy as dependencies. I haven't used uncertainties[arrays]. I feel like that is probably common (if not the most correct).
  • pip and PyPI do not handle upper bounds very well. It is not possible to go back and add an upper bound like you could with conda or Linux distribution packages. So if uncertainties 3.2.0 pins numpy to <2, pip will still install uncertainties 3.1.7 and numpy 2.0 together if you do not pin uncertainties[arrays]>=3.2.0. I am not sure which logic it uses to break the tie when two packages' latest versions preclude the latest version of the other.
  • The test coverage of unumpy is around 94% and ruff has a checker specifically for the numpy 2 code removals. So I feel pretty good about the use of actual numpy API calls. However, uncertainties also tries to mimic the numpy API and there is not a test that systematically loops over every function and compares its behavior to the numpy equivalent, so there could be some drift (Review numpy API coverage #224).

@newville
Copy link
Member

@wshanks @andrewgsavage Thanks! I understand that upper bounds on versions are not ideal in general and does not always work well.

Still, I might suggest that we merge both #225 and this PR to limit numpy < 2, at least for now, and then be ready to lift this restriction soon. (I am also +1 on #223 and #226). I think we can be cautious about supporting Numpy 2 for a little while, even if we believe it will mostly work just fine.

As with #224 and the recently re-pinged Issue #114, I think there are a lot of "legacy corner cases" in the unumpy interface, - including matrix support - that may need to be carefully checked with NumPy 2.

But: I could be convinced to just go for it and allow Numpy 2 and (try to) deal with any consequences.

@andrewgsavage
Copy link
Contributor Author

andrewgsavage commented May 16, 2024 via email

@newville
Copy link
Member

@andrewgsavage I am OK with closing this. We can re-open if needed.

@newville newville closed this May 16, 2024
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.

3 participants