BUG: fix compatibility with numpy 2.0 copy semantics by neutrinoceros · Pull Request #16142 · astropy/astropy

neutrinoceros · 2024-03-03T14:12:14Z

Description

This adresses an new incompatibility with numpy dev:
np.array(..., copy=False) now raises a ValueError if a copy cannot be avoided. The suggested replacement, implemented in scikit-learn by the author of the breaking change, is to use np.asarray(...) instead, which will only create a copy if necessary, as desired.

xref numpy/numpy#25168

Note that this doesn't fix devdeps by itself: there's also an upstream issue in pyerfa,~ and another incompat with Unit that I'll need to bisect, and which I think can be addressed separately.~
update: turns out this other incompatibility was an artifact of my first attempt at this patch, not an actual issue.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-03-03T14:12:43Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

neutrinoceros · 2024-03-03T14:26:57Z

Whoops, the whole CI just broke with

TypeError: unsupported operand type(s) for *: 'Unit' and 'float'

Incidentally, this is the "other error" that I saw as I tested against numpy dev locally, but I didn't realise that it was actually caused by this patch. So the good news is that we probably have only one problem to solve here ! The bad news is that it's worth than I thought.

rgommers · 2024-03-03T15:33:52Z

The bad news is that it's worth than I thought.

This was an unintended regression in numpy; it should be emitting a warning if copy is missing from the signature rather than raising a TypeError. Should be fixed within the next 24 hours (hopefully sooner). Upstream issue is numpy/numpy#25916.

neutrinoceros · 2024-03-03T15:42:54Z

Thanks for jumping in @rgommers !
Actually I wasn't talking about this specific aspect, I just got confused when I blindly replaced np.array(..., copy=False) with np.asarray(...) without realising that some arguments (ndmin, subok) don't make sense there.

As for the TypeError VS warning, it doesn't make a difference on our side since warnings are treated as errors in CI, and in both case it's very useful information. Still, a warning would indeed be nicer for end users. Any way, thank you for all your work on numpy 2 !

mhvk

Thanks! Looking through the changes,I think two things would simplify this:

I think we should define COPY_IF_NEEDED = False if NUMPY_LT_2_0 else None in numpycompat and just import that.
The __array__ implementations can all have copy=COPY_IF_NEEDED as an argument, independent of numpy version.

mhvk · 2024-03-03T16:37:59Z

+            """
+            Array representation of the kernel.
+            """
+            return self._array


Separately, I'm actually not sure this implementation is correct, I think it should be return self._array.astype(dtype=dtype, copy=copy) but then the default would have to be False (which I think is fine).

I've added this to my personal TODO list and will make sure to address it in a follow up PR. Thanks !

also reported in #16168

rgommers · 2024-03-03T17:30:30Z

I think two things would simplify this:

I'll note that that is pretty much exactly what we did in SciPy: scipy/scipy#20172. Looks pretty clean that way.

neutrinoceros · 2024-03-03T17:34:29Z

Thanks @mhvk ! Right now I'm trying to eliminate any regressions seen in CI but I'll get to your suggestions next !

neutrinoceros · 2024-03-03T18:00:04Z

One regression to go before refactoring. Not sure I'll have time to finish this today but I should see the end of it tomorrow.

neutrinoceros · 2024-03-03T20:21:54Z

I'm not done refactoring, but I'm going to leave this in a functioning state for today.

mhvk · 2024-03-03T20:35:08Z

I'm not done refactoring, but I'm going to leave this in a functioning state for today.

Sure! It is looking good - just continue to use COPY_IF_NEEDED liberally!

neutrinoceros · 2024-03-04T21:29:39Z

@mhvk I will only have very limited access to a keyboard (possibly none) until Wednesday so feel free to change this bit.

mhvk · 2024-03-04T21:45:46Z

OK, will push up changes (they turn out to be minimal, thankfully), and then try to get this in. Thanks for all of this - it was non-trivial!

mhvk

Approving. Since this now introduces no API changes at all, i.e., just accounts for the change in np.array in numpy 2.0, I'll go ahead and approve and set ready to merge. @neutrinoceros will open a new issue/PR with plans for how to do this going forward.

pllim · 2024-03-04T21:56:23Z

Are you sure you want to auto-merge? numpy-dev job is not required to pass before it kicks in.

UPDATE: devdeps failed -- I disabled it.

pllim · 2024-03-04T22:03:23Z

Hmm... Is this related?

2024-03-04T22:00:37.4696084Z _____________________________ test_array_api_init ______________________________�[0m
2024-03-04T22:00:37.4696733Z 
2024-03-04T22:00:37.4696944Z     @pytest.mark.filterwarnings(
2024-03-04T22:00:37.4697774Z         "ignore: The numpy.array_api submodule is still experimental. See NEP 47."
2024-03-04T22:00:37.4698595Z     )
2024-03-04T22:00:37.4698990Z     def test_array_api_init():
2024-03-04T22:00:37.4699537Z >       import numpy.array_api as np_api
2024-03-04T22:00:37.4700464Z E       ModuleNotFoundError: No module named 'numpy.array_api'�[0m
2024-03-04T22:00:37.4701099Z 
2024-03-04T22:00:37.4702554Z astropy/units/tests/test_quantity_array_methods.py�[0m:649: ModuleNotFoundError

MridulS · 2024-03-04T22:05:10Z

@pllim that should be fixed with #16150

mhvk · 2024-03-05T00:02:58Z

I knew it was going to fail given #16150, but indeed, given that that is the simpler PR, might as well check here that we are now all green again. So, will rebase and then let the tests run their course. Thanks, @pllim, for thinking this through better!

mhvk · 2024-03-05T00:50:15Z

Yay, devdeps is green again! Merging...

… semantics

Backport PR #16142: BUG: fix compatibility with numpy 2.0 copy semantics

neutrinoceros requested review from a team, larrybradley and taldcroft as code owners March 3, 2024 14:12

github-actions Bot added table nddata units modeling convolution uncertainty labels Mar 3, 2024

neutrinoceros marked this pull request as draft March 3, 2024 14:27

neutrinoceros mentioned this pull request Mar 3, 2024

BUG: fix compatibility with numpy 2 for optional array copy (np.array(..., copy=False)) liberfa/pyerfa#135

Closed

neutrinoceros force-pushed the numpy2/array_copy_False branch 3 times, most recently from 72b5086 to 217a906 Compare March 3, 2024 15:30

neutrinoceros force-pushed the numpy2/array_copy_False branch from d5bc025 to aa7214e Compare March 3, 2024 17:09

mhvk reviewed Mar 3, 2024

View reviewed changes

neutrinoceros force-pushed the numpy2/array_copy_False branch 2 times, most recently from fdbf75c to cf59a18 Compare March 3, 2024 19:10

neutrinoceros force-pushed the numpy2/array_copy_False branch from 958a22f to 7b23323 Compare March 3, 2024 20:22

neutrinoceros force-pushed the numpy2/array_copy_False branch 2 times, most recently from 7061621 to 4f137de Compare March 4, 2024 11:41

mhvk approved these changes Mar 4, 2024

View reviewed changes

mhvk enabled auto-merge March 4, 2024 21:54

pllim disabled auto-merge March 4, 2024 22:01

neutrinoceros and others added 3 commits March 4, 2024 19:03

BUG: fix compatibility with numpy 2.0 copy semantics

ca0c725

TST: fix a broken test

45ae0e5

Let Quantity use COPY_IF_NEEDED internally

6068539

mhvk force-pushed the numpy2/array_copy_False branch from df9ab49 to 6068539 Compare March 5, 2024 00:03

mhvk merged commit d32a06a into astropy:main Mar 5, 2024

This comment was marked as resolved.

Sign in to view

lumberbot-app Bot added the Still Needs Manual Backport label Mar 5, 2024

mhvk added a commit to mhvk/astropy that referenced this pull request Mar 5, 2024

Backport PR astropy#16142: BUG: fix compatibility with numpy 2.0 copy…

3104417

… semantics

pllim added a commit that referenced this pull request Mar 5, 2024

Merge pull request #16157 from mhvk/auto-backport-of-pr-16142-on-v6.0.x

8edd38c

Backport PR #16142: BUG: fix compatibility with numpy 2.0 copy semantics

pllim mentioned this pull request Mar 5, 2024

Backport PR #16142: BUG: fix compatibility with numpy 2.0 copy semantics #16157

Merged

1 task

pllim removed the Still Needs Manual Backport label Mar 5, 2024

neutrinoceros deleted the numpy2/array_copy_False branch March 5, 2024 06:27

pllim mentioned this pull request Mar 15, 2024

Release 6.1 when numpy 2.0 is released? #16200

Closed

registerrier mentioned this pull request May 16, 2024

Introduce COPY_IF_NEEDED to allow compatibility with numpy 2.0 gammapy/gammapy#5264

Merged

namurphy mentioned this pull request Jan 15, 2025

TST: decoupling stable CI and incompatibilities discovery (let's use lock files) #17596

Open

Conversation

neutrinoceros commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Mar 3, 2024

Uh oh!

neutrinoceros commented Mar 3, 2024

Uh oh!

rgommers commented Mar 3, 2024

Uh oh!

neutrinoceros commented Mar 3, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk Mar 3, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Mar 3, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgommers commented Mar 3, 2024

Uh oh!

neutrinoceros commented Mar 3, 2024

Uh oh!

neutrinoceros commented Mar 3, 2024

Uh oh!

neutrinoceros commented Mar 3, 2024

Uh oh!

mhvk commented Mar 3, 2024

Uh oh!

neutrinoceros commented Mar 4, 2024

Uh oh!

mhvk commented Mar 4, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Mar 4, 2024

Uh oh!

MridulS commented Mar 4, 2024

Uh oh!

mhvk commented Mar 5, 2024

Uh oh!

mhvk commented Mar 5, 2024

Uh oh!

This comment was marked as resolved.

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

neutrinoceros commented Mar 3, 2024 •

edited

Loading

pllim commented Mar 4, 2024 •

edited

Loading