8000 BUG: fix compatibility with numpy 2.0 copy semantics by neutrinoceros · Pull Request #16142 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

BUG: fix compatibility with numpy 2.0 copy semantics #16142

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 5, 2024
Merged

BUG: fix compatibility with numpy 2.0 copy semantics #16142

merged 3 commits into from
Mar 5, 2024

Conversation

neutrinoceros
Copy link
Contributor
@neutrinoceros neutrinoceros commented Mar 3, 2024 •
edited
Loading

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.

@neutrinoceros neutrinoceros requested review from taldcroft, a team and larrybradley as code owners March 3, 2024 14:12
@github-actions GitHub Actions
Copy link
Contributor
github-actions bot commented Mar 3, 2024

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
Copy link
Contributor Author

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.

@neutrinoceros neutrinoceros marked this pull request as draft March 3, 2024 14:27
@neutrinoceros neutrinoceros mentioned this pull request Mar 3, 2024
Closed
@neutrinoceros neutrinoceros force-pushed the numpy2/array_copy_False branch 3 times, most recently from 72b5086 to 217a906 Compare March 3, 2024 15:30
@rgommers
Copy link
Contributor
rgommers commented Mar 3, 2024

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
Copy link
Contributor Author

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 !

@neutrinoceros neutrinoceros force-pushed the numpy2/array_copy_False branch from d5bc025 to aa7214e Compare March 3, 2024 17:09
mhvk
mhvk reviewed Mar 3, 2024
View reviewed changes
8000 Copy link
Contributor
@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

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

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

astropy/convolution/core.py Outdated
"""
Array representation of the kernel.
"""
return self._array
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also reported in #16168

@rgommers
Copy link
Contributor
rgommers commented Mar 3, 2024

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
Copy link
Contributor Author

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

@neutrinoceros
Copy link
Contributor Author

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 neutrinoceros force-pushed the numpy2/array_copy_False branch 2 times, most recently from fdbf75c to cf59a18 Compare March 3, 2024 19:10
@neutrinoceros
Copy link
Contributor Author

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

@neutrinoceros neutrinoceros force-pushed the numpy2/array_copy_False branch from 958a22f to 7b23323 Compare March 3, 2024 20:22
@mhvk
Copy link
Contributor
mhvk commented Mar 3, 2024

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 neutrinoceros force-pushed the numpy2/array_copy_False branch 2 times, most recently 8000 from 7061621 to 4f137de Compare March 4, 2024 11:41
@neutrinoceros
Copy link
Contributor Author

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

@mhvk
Copy link
Contributor
mhvk commented Mar 4, 2024

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
mhvk approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor
@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

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.

@mhvk mhvk enabled auto-merge March 4, 2024 21:54
@pllim
Copy link
Member
pllim commented Mar 4, 2024 •
edited
Loading

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 pllim disabled auto-merge March 4, 2024 22:01
@pllim
Copy link
Member
pllim commented Mar 4, 2024

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
Copy link
Contributor
MridulS commented Mar 4, 2024

@pllim that should be fixed with #16150

@mhvk
Copy link
Contributor
mhvk commented Mar 5, 2024

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 mhvk force-pushed the numpy2/array_copy_False branch from df9ab49 to 6068539 Compare March 5, 2024 00:03
@mhvk
Copy link
Contributor
mhvk commented Mar 5, 2024

Yay, devdeps is green again! Merging...

@mhvk mhvk merged commit d32a06a into astropy:main Mar 5, 2024
@lumberbot-app Lumberbot (App)

This comment was marked as resolved.

Sign in to view
mhvk added a commit to mhvk/astropy that referenced this pull request Mar 5, 2024
@mhvk
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
@pllim
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 pllim mentioned this pull request Mar 5, 2024
Merged
1 task
@neutrinoceros neutrinoceros deleted the numpy2/array_copy_False branch March 5, 2024 06:27
This was referenced Mar 6, 2024
Closed
Merged
Open
Merged
Merged
@pllim pllim mentioned this pull request Mar 15, 2024
Closed
@registerrier registerrier mentioned this pull request May 16, 2024
Merged
@namurphy namurphy mentioned this pull request Jan 15, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk approved these changes

@taldcroft taldcroft Awaiting requested review from taldcroft taldcroft is a code owner

@larrybradley larrybradley Awaiting requested review from larrybradley larrybradley is a code owner

Assignees
No one assigned
Labels
convolution coordinates modeling nddata no-changelog-entry-needed numpy-dev table time uncertainty units utils 💤 backport-v6.0.x on-merge: backport to v6.0.x
Projects
None yet
Milestone
v6.0.1
Development

Successfully merging this pull request may close these issues.
5 participants
@neutrinoceros @rgommers @mhvk @pllim @MridulS
0