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

[Numpy2] Support for numpy==2.0.0 (and pin iris >=3.11) #2395

Merged
merged 80 commits into from
Nov 11, 2024
Merged

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Apr 12, 2024

Description

This PR introduces the changes we need to support Numpy 2.0 (these changes were collected from running the tests, so they may not be 100% exhaustive, but given that we have 95% test coverage, I'd say they are 95% exhaustive; we can deal with missed cases as they come along, if any; I would not use the ruff plugin test Numpy recommends to migrate, since that will introduce a lot of style changes that we may want to make when we include ruff as our linter). Most changes are in our testing suite, but there are a couple in the source code; these are backwards compatible changes, ie all tests pass when running with numpy=1.26.4

The only test that fails (both for Numpy2 and numpy legacy) is related to the pins on pandas, that need to go since we need to use the latest pandas for environment compatibility, see #2459 ; I'll try fix that issue here, since it's related to (but not a direct consequence of) Numpy2.

Closes #2460
Closes #2459

iris issue to monitor SciTools/iris#6043

-> which that is already in iris==3.11, which leads to

TODO pin iris>=3.11

-> pinned in commits dddc083 and 858ae11

The second issue closure is more of a brush it under the carpet via a pytest xfail, the original issue that kept creeping up is #2305 and the actual issue is with Pandas, and listed in pandas-dev/pandas#57002 - we can wait on its fix (and have us test quietness via the test xfail) or @schlunma can propose a more elegant solution in a separate PR?

Historical guff

OK @ESMValGroup/technical-lead-development-team we finally have a correct set of tests with the latest Numpy=2.0.0 (stable) - don't mind the ones related to pandas, I had to unpin it to gte pandas=2.2.2 built with Numpy 2.0 (otherwise the environment wouldn't solve) -> we have some work to do 👍 https://github.com/ESMValGroup/ESMValCore/actions/runs/9582595825/job/26421917358?pr=2395

Numpy 2.0 is impending: it is now fully out (see https://anaconda.org/conda-forge/numpy ); resources https://anaconda.org/conda-forge/numpy/files and https://github.com/numpy/numpy/releases - they recommend a test with the RC and list the changes roadmap in https://numpy.org/devdocs/numpy_2_0_migration_guide.html so we can test ourselves. Note: this branch should not be merged! If everything works out fine, we'll just pick up numpy 2.0 as per normal. ATTN: @ESMValGroup/technical-lead-development-team

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.66%. Comparing base (f74a2ad) to head (24f2549).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2395      +/-   ##
==========================================
- Coverage   94.76%   94.66%   -0.11%     
==========================================
  Files         251      251              
  Lines       14280    14285       +5     
==========================================
- Hits        13533    13523      -10     
- Misses        747      762      +15     

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

@valeriupredoi
Copy link
Contributor Author

@bouweandela
Copy link
Member

xref Unidata/cftime#318

@valeriupredoi
Copy link
Contributor Author

Spot on @bouweandela 🍻 Fortunately there is a mature PR for it Unidata/cftime#319

@valeriupredoi
Copy link
Contributor Author

K then, just waiting for a new cftime to be released

@valeriupredoi valeriupredoi changed the title [DNM][Test Only] Test with numpy==2.0.0rc1 [DNM][Test Only] Test with numpy==2.0.0rc2 May 27, 2024
@valeriupredoi
Copy link
Contributor Author

still royally hosed even with rc2

@valeriupredoi valeriupredoi changed the title [Numpy2] Support for numpy==2.0.0 [Numpy2] Support for numpy==2.0.0 (and pin iris >=3.11) Nov 11, 2024
@valeriupredoi
Copy link
Contributor Author

@schlunma would you have a bit of time and come back to finishing the review on this, please, bud? Iris 3.11 is fully out and tested for here, and once we get this into main, then I can finish #2566 and with that we'll have support for the latest Python 3.13 too - we'll shipshape and completely up to date with our dependencies 🍺

@valeriupredoi
Copy link
Contributor Author

also CC @bouweandela for a final review and merge, after Manu is happy 🍺

esmvalcore/preprocessor/_regrid_esmpy.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
Comment on lines 172 to 176
# explicitly casting to int32 since without it, in Numpy 2.0
# one gets OverflowError: Python integer 360 out of bounds for int8
# see notes https://numpy.org/devdocs/numpy_2_0_migration_guide.html
src_points_with_convex_hull[-2 * n_hull:-n_hull, 1] -= np.int32(360)
src_points_with_convex_hull[-n_hull:, 1] += np.int32(360)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I still don't understand this. When I run the tests (numpy < 2), src_points_with_convex_hull is an array with dtype float64 as expected by the CMOR standards. Numpy >= 2 should NOT change that. It would be wrong if this array is of dtype int.

I just tested this locally with dummy arrays and it works with 360 and 360.0 (the latter would be "more" correct). Maybe this was just a problem of an early numpy version. Would it be able to test this again?

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Thanks V, great that we will have support for numpy 2 soon! A couple of comments from me.

@valeriupredoi
Copy link
Contributor Author

many thanks Manu! Let me try change back the syntax in esmvalcore/preprocessor/_regrid_unstructured.py

@valeriupredoi
Copy link
Contributor Author

OK for some reason my two comments are not showing up on the main page - was saying that that test still fails with numpy=2.1.3 (latest), see test fail - numpy is looking at 360 as an int8 and spitting an Overflow, hence my workaround to make it int32 and avoid the Overflow - it could be an float64 like any coord point in CMOR - just anything to avoid silly numpy thinking it's int8

@valeriupredoi
Copy link
Contributor Author

not just there yet Manu - numpy 2 is a bit of an 🫏 😁

@valeriupredoi
Copy link
Contributor Author

green green green 🟢 like at WEC races 🏁 - cheers for fixing, Manu!

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

All green now 🟢 Thanks V!!

@valeriupredoi
Copy link
Contributor Author

I'll merge now, if that's OK with you - will get @bouweandela to review the Python 3.13 PR #2566

@valeriupredoi valeriupredoi merged commit 4b83308 into main Nov 11, 2024
6 checks passed
@valeriupredoi valeriupredoi deleted the numpy2-dev branch November 11, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request iris311 needs iris 3.11 stable release Numpy2 testing
Projects
None yet
3 participants