API: align default value for copy arguments with numpy at runtime (copy=False raises an error if copies cannot be avoided) #16181

neutrinoceros · 2024-03-08T07:12:28Z

Description

This is based off #16166 and #16170 and adds a minor but crucial API change to align the default value of our copy arguments with whatever numpy API is installed. See #16167 for the broader context.

Alternative to #16174:
Passing copy=False immediately breaks if copies cannot be avoided (in alignment with numpy 2.0)

This is meant to be a small self-contained PR that we can ask downstream stakeholders to test from, but this will only make sense once #16166 and #16170 are merged. At that point, I'll call for feedback on the dev mailing list.

Close #16167

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-08T07:12:55Z

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.

github-actions · 2024-03-08T07:12:57Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

neutrinoceros · 2024-03-08T07:25:38Z

Note to self (and reviewers): if this is turns out preferred over #16174, I'll need to update docstrings where due. I'm avoiding doing it now so rebasing stays as easy a possible.

pllim · 2024-03-08T19:21:11Z

FWIW, I vote for this one but you should wait a bit more before pushing more changes, just in case. Thanks!

pllim · 2024-03-08T19:21:55Z

Well, as long as behavior does not change for numpy 1.x, is that correct?

neutrinoceros · 2024-03-09T07:33:52Z

That's right. The title is maybe misleading, but this patch just delegates all copy decisions to numpy, which, with numpy 1 installed, isn't a change at all.

taldcroft · 2024-03-14T11:06:05Z

@neutrinoceros - I took some time to try getting up to speed on the huge refactors to address the numpy 2.0 copy issue. From what I can see this all looks good and aligns with the (good) decision to delegate the default behavior to numpy. This PR continues that effort in a few places.

My question here is process. The PR description mentions that this PR is something to "ask downstream stakeholders to test from". I would assume you mean to merge this to main and let people test with astropy dev (main).

Overall everything I see in this PR looks good and uncontroversial at this point. However there is a conflict that needs to be resolved.

neutrinoceros · 2024-03-14T14:13:25Z

My question here is process. The PR description mentions that this PR is something to "ask downstream stakeholders to test from". I would assume you mean to merge this to main and let people test with astropy dev (main).

Actually what I meant was to offer downstream users to test the unmerged branch, because I was initially more worried about breaking stuff than I am now.

In hindsight, merging first, as you suggest, would probably be easier for everyone.

Since I haven't heard back from any one yet, I'll assume nobody's currently relying on this branch's state, so let me rebase to fix the one conflict !

neutrinoceros · 2024-03-14T16:13:24Z

@taldcroft there was also the fact that I initially conceived downstream testing as a mean to choose our API change (with #16174 as the alternative), but I think I convinced myself that #16174 wasn't really a viable option while implementing it.
All in all, and unless someone objects to it, my current recommendation would be to merge this one, close #16174, and issue and update on astropy-dev about testing main instead of this PR.

mhvk

See long in-line comment about the changelog-entry, and a general worry that something like Table(..., copy=None) may not work...

It may be best, however, to worry about those other things in a different issue/PR. 6.1 is not that far away and it might be good if people's CI can start testing against this...

mhvk · 2024-03-14T17:56:41Z

docs/changes/16181.other.rst

+Public APIs that expose a ``copy`` argument and that previously set ``False``
+as a default value now use ``None`` instead if Numpy 2.0 or newer is installed.
+Default behaviour is unaffected, however explicitly passing ``copy=False`` will
+now raise an error if copying cannot be avoided.


Maybe be explicit and say "will raise an error under numpy 2.0 or newer if ...".

It is a bit double, but makes clear that numpy 1.x is not affected.

Actually, looking at the list that follows, I first thought this cannot be true - see comment on the differential - but then worried more generally about it. I think this change really affects everything, including SkyCoord, for which copy=False would just get passed down until it hits the representations. So, I think we should rephrase this as a new paragraph (and really this needs to be in some form of what's new for 6.1 too...), like

Public APIs that expose a ``copy`` argument and that previously set ``False`` as a default value now use ``None`` instead if Numpy 2.0 or newer is installed. This is because in numpy 2.0, the meaning of the ``copy`` argument has changed, with ``copy=False`` now indicating that a copy should never be made, while ``copy=None`` is used for the previous meaning of "avoid a copy if possible". While this change ensures the default behaviour of astropy has not changed, code that explicitly passes ``copy=False`` to many of astropy's classes will need to be adjusted. This includes:

(and then the list should not include CylindricalRepresentation but should probably include SkyCoord)

A logical question here is whether we should include how to adjust it. Should we link to astropy.utils.compat.COPY_IF_NEEDED?

A more pragmatic question is whether we need tests to check that things do not percolate badly. E.g., I'm not sure that Table really belongs in the list.

A logical question here is whether we should include how to adjust it. Should we link to astropy.utils.compat.COPY_IF_NEEDED?

I had thought the right answer is to not pass a copy=... argument at all, deferring to the default. But this may be upsetting to people that want to be explicit and not rely on a default.

I had also wondered about copy=None. The table says that the behavior is formally undefined but in practice for numpy 1.x it is the same as False.

I'll adjust the note, and I agree it belongs in the what's new.

A logical question here is whether we should include how to adjust it. Should we link to astropy.utils.compat.COPY_IF_NEEDED?

As far as I understand, this module isn't considered public API (we just removed PYTHON_LT_* constants from it without a warning), so I wouldn't want to encourage downstream to rely on it. That said, we could recommend copying its implementation (the tricky part, I think, is just in how to properly check numpy's version at runtime using packaging.version). What do you think ?

A more pragmatic question is whether we need tests to check that things do not percolate badly.

Could you clarify what you mean here ? From my standpoint, the "percolating badly" scenario would be #16174 😅 .

Oops, going on a race condition with @taldcroft .

I had thought the right answer is to not pass a copy=... argument at all, deferring to the default. But this may be upsetting to people that want to be explicit and not rely on a default.

Well it is documented as mostly for internal usage, and people who want to be explicit about not copying have no recourse to do so in numpy 1, so I'm not too worried.

I had also wondered about copy=None. The table says that the behavior is formally undefined but in practice for numpy 1.x it is the same as False.

unless you're using a type checker, but essentially yes ! We could even recommend using a plain None as a backward compatible and cheap replacement.

I don't think one can just use copy=None on all numpy versions:

In [2]: np.__version__ Out[2]: '1.24.2' In [3]: np.array([1, 2, 3], copy=None) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) Cell In [3], line 1 ----> 1 np.array([1, 2, 3], copy=None) ValueError: NoneType copy mode not allowed.

This is also what I meant by tests. Since we use copy=False basically only internally, I'm not so sure that we cover most cases users might do. I think we should be OK, though, since in most cases one would not be asking for a dtype change, so, say, SkyCoord(ra_array, dec_array, copy=False) should just continue to work.

Anyway, bottom line to me seems that we should just try to make sure this is out when numpy 2.0 comes out, so that we can get reports from users. Indeed, perhaps this is the one time that we should ask people to run their test suites on a point release...

Actually I think there are conflicting desires here: if we're adding a what's new, then backporting (along with supporting PRs) becomes impossible (patch releases don't have what's news).
So, the easiest way to resolve this tension would probably be to not have a what's new, and instead make the changelog entry even longer. As long as we make sure that the release team is aware of the importance of this patch and can properly emphasize it as part of our numpy 2.0 compatibility layer, it should be good ?

Yes, makes sense!

I really think we should only release all these changes in 6.1.0 and have a What's New.

If there is a chance that numpy 2.0 will get released before early May then we should move the astropy release forward. Having these changes in a bug fix release is (IMO) totally incompatible with our policy for such releases. Despite a lot of effort and review, I think it is credible that the numpy 2.0 compatibility changes will break some existing code somewhere. Users don't expect this for a bug fix release.

OK, that makes sense too. So, we basically will make clear that astropy 6.0 requires numpy < 2.

I had vaguely considered the idea of an early 6.1 as well - seems worth a new issue! #16200

ok then I'll make a what's new from the changelog

mhvk · 2024-03-14T18:03:52Z

astropy/coordinates/representation/cylindrical.py

    base_representation = CylindricalRepresentation

-    def __init__(self, d_rho, d_phi=None, d_z=None, copy=False):
+    def __init__(self, d_rho, d_phi=None, d_z=None, copy=COPY_IF_NEEDED):


This is actually really weird - why would the Differential have copy=False as a default when the representation does not (and indeed all representations and differentials have copy=True, though mostly implicitly via BaseRep*; but a concrete example is PhysicsSphericalDifferential).

It maybe should be in a different PR, but let's at least not mention this in the changelog entry...

How about I revert this bit here and open a dedicated issue for it ?

Yes, good idea, let's just have a PR that changes it to True - this is the default for essentially all astropy classes (except Masked, where False makes much more sense).

done in #16198

(and reverted here)

No, it shouldn't be reverted - just use copy=True here!

Oops, sorry, commenting on something that's out of date! All OK.

mhvk

I'm happy with this! I have a suggestion to simplify the version check but OK to leave as is too.

mhvk · 2024-03-15T13:57:42Z

docs/changes/16181.other.rst

+instances of ``False`` be replaced with a ``COPY_IF_NEEDED`` constant defined
+as follow
+
+    def NUMPY_GE_2_0() -> bool:


Can we just recommend a simpler version?

COPY_IF_NEEDED = False if np.__version__[0] == "1" else None

Given how these compatibility layers tend to be left to rot in unmaintained software, I'm slightly uneasy to recommend something that would break in the event of numpy going to 3.0. I'm also hesitant to recommend more than one way to do it, which is why I went with the more robust alternative.

hum, on second thought I think this would only break if there was a Numpy 10.0 ... and by then, anything that still has compatibility code for Numpy 1.x is likely to be broken anyway so, let's just roll with it !

and on third thought, we can make it even more portable at no real addtional cost:

COPY_IF_NEEDED = False if np.__version__.startswith("1.") else None

neutrinoceros · 2024-03-15T15:16:44Z

I moved the large changelog fragment to a What's New section, and replaced the changelog with a one liner version of its past self. Should be good now ?

neutrinoceros · 2024-03-15T15:41:27Z

Failure in devdeps seem unrelated but it's still kinda scary (and unfamiliar to me).

mhvk · 2024-03-15T15:46:30Z

docs/changes/16181.other.rst

@@ -0,0 +1,3 @@
+Change the default value of ``copy`` arguments in public APIs from ``False`` to
+``None`` if Numpy 2.0 or newer is installed.
+For details, see :ref:`What's New in Astropy 6.1?<_whatsnew-6.1-copy-semantics>`_


I think we generally try to avoid putting links in changelog fragments (because they are more likely to get out of date), but I'm not sure whether this one is OK - let's ping @pllim!

Yeah:

That ref is wrong. Should have used whatsnew-6.1-copy-semantics (without whitespace maybe because RTD has a cryptic warning).

I also remember some past pain where we linked to something in What's New but then the link broke after release. But I don't remember exactly what broke. @saimn , do you?

p.s. Maybe it is not the change log fragment but the what's new. See #16181 (comment)

Ah, so the problem is having both :ref: and trailing _.

Ah, that would do it.

thank you ! should be fixed now !

mhvk

Looks great, modulo the one worry about the link in the changelog fragment. Approving assuming that is actually OK.

pllim · 2024-03-15T17:55:00Z

Re: devdeps -- Looks like transient network error but I restarted it, just in case. I also added Extra CI to play safe.

mhvk · 2024-03-15T18:29:39Z

devdeps failure is yet another bunch of str ufuncs being added to numpy 2.0 (probably the last bunch to come...)

doc link test I didn't quite have the patience to try to understand what exactly went wrong - rather a lot of less relevant information to parse through...

pllim · 2024-03-15T18:39:28Z

linkcheck isn't related, so you can ignore that one.

RTD warning is related:

whatsnew/index
WARNING: :347: (WARNING/2) Mismatch: both interpreted text role prefix and reference suffix.

neutrinoceros · 2024-03-16T07:58:24Z

@mhvk thanks ! I fixed the devdeps failure in #16205

neutrinoceros · 2024-04-02T12:05:44Z

rebased to fix a merge conflict

mhvk · 2024-04-02T13:20:58Z

failures due to #16251

pllim · 2024-04-03T16:40:25Z

docs/changes/16181.other.rst

@@ -0,0 +1,3 @@
+Change the default value of ``copy`` arguments in public APIs from ``False`` to
+``None`` if Numpy 2.0 or newer is installed.
+For details, see :ref:`What's New in Astropy 6.1?<_whatsnew-6.1-copy-semantics>`


This failed RTD build with a warning:

WARNING: undefined label: '_whatsnew-6.1-copy-semantics'

Also I don't think intersphinx is going to last forever in What's New as we "archive" the page at release time. For example, please see https://github.com/astropy/astropy/blob/main/docs/whatsnew/6.0.rst

Thanks for pointing it out, how can I fix this ? Should I just refer to the what's new page in text and avoid any linking ?

I am okay with a mention without any link. What do you think, @mhvk ?

Agreed, let's not over{th|l}ink.

thanks, I removed the link ! Let me know if it's ok now

… numpy)

docs/changes/16181.other.rst

Co-authored-by: P. L. Lim <[email protected]>

neutrinoceros · 2024-04-04T07:29:29Z

I really can't understand how docs are still failing (I can't even find where the error is in the log)
Other failures are unrelated and already addressed.

astrofrog · 2024-04-04T07:32:52Z

I can try and investigate this morning

astrofrog · 2024-04-04T09:49:18Z

The RTD docs build is fine, the one that is failing is the link check, but I think these are unrelated transient issues. The log is hard to parse but by grepping ' broken ' I find:

2024-04-03T22:15:47.3818420Z (io/fits/usage/headers: line  336) broken    https://heasarc.gsfc.nasa.gov/docs/heasarc/ofwg/docs/ofwg_recomm/r13.html - ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response'))
2024-04-03T22:15:47.3895910Z (api/astropy.units.format.OGIP: line    2) broken    https://heasarc.gsfc.nasa.gov/docs/heasarc/ofwg/docs/general/ogip_93_001/ - ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response'))
2024-04-03T22:15:47.4963577Z (io/fits/appendix/faq: line  841) broken    https://heasarc.gsfc.nasa.gov/docs/software/fitsio/c/c_user/node97.html - ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response'))
2024-04-03T22:15:47.5889324Z (io/fits/appendix/faq: line  139) broken    https://heasarc.gsfc.nasa.gov/docs/software/ftools/fitsverify/ - ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response'))
2024-04-03T22:15:47.6081347Z (io/fits/appendix/faq: line  583) broken    https://heasarc.gsfc.nasa.gov/fitsio/fitsio.html - ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response'))
2024-04-03T22:15:57.2591337Z (   timeseries/io: line    9) broken    https://tess.gsfc.nasa.gov/ - ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response'))

All are .nasa.gov-related. We should perhaps (in a separate PR) enable quiet mode for the link check as suggested in sphinx-doc/sphinx#8791 as this will then only show the broken links, not all the valid ones.

astrofrog

Failures are unrelated, looks good to me!

github-actions bot added table time units coordinates modeling utils utils.masked labels Mar 8, 2024

neutrinoceros mentioned this pull request Mar 8, 2024

API: align default value for copy arguments with numpy at runtime (warn about copy=False) #16174

Closed

1 task

neutrinoceros changed the title ~~API: align default value for copy arguments with numpy at runtime~~ API: align default value for copy arguments with numpy at runtime (copy=False raises an error if copies cannot be avoided) Mar 8, 2024

neutrinoceros mentioned this pull request Mar 8, 2024

Going forward after numpy 2.0's change of copy semantics #16167

Closed

pllim added this to the v6.1.0 milestone Mar 8, 2024

neutrinoceros mentioned this pull request Mar 9, 2024

RFC: refactor copy argument sanitizing into a unified compat function #16170

Merged

1 task

neutrinoceros force-pushed the api/copy_if_needed_clean_break branch 4 times, most recently from 7299fdf to 5a76d55 Compare March 11, 2024 13:27

neutrinoceros marked this pull request as ready for review March 11, 2024 13:43

neutrinoceros requested a review from taldcroft as a code owner March 11, 2024 13:43

neutrinoceros force-pushed the api/copy_if_needed_clean_break branch from 5a76d55 to e7d531d Compare March 14, 2024 14:15

mhvk reviewed Mar 14, 2024

View reviewed changes

neutrinoceros mentioned this pull request Mar 14, 2024

BUG/API: Changed the default value of the copy argument in CylindricalDifferential.__init__ from False to True #16198

Merged

1 task

neutrinoceros force-pushed the api/copy_if_needed_clean_break branch from af47249 to 0a8d367 Compare March 15, 2024 07:33

mhvk approved these changes Mar 15, 2024

View reviewed changes

neutrinoceros force-pushed the api/copy_if_needed_clean_break branch from 0a8d367 to f8b1b9a Compare March 15, 2024 15:15

mhvk reviewed Mar 15, 2024

View reviewed changes

mhvk approved these changes Mar 15, 2024

View reviewed changes

pllim added the Extra CI Run cron CI as part of PR label Mar 15, 2024

neutrinoceros force-pushed the api/copy_if_needed_clean_break branch 2 times, most recently from 949584e to c58abc2 Compare March 16, 2024 07:57

neutrinoceros force-pushed the api/copy_if_needed_clean_break branch from c58abc2 to f5f3fff Compare April 2, 2024 12:05

pllim reviewed Apr 3, 2024

View reviewed changes

neutrinoceros added 2 commits April 3, 2024 22:02

API: align default value for copy arguments with numpy at runtime

f4525c6

API: change meaning of copy=False in astropy APIs (align with runtime…

8016bd2

… numpy)

neutrinoceros force-pushed the api/copy_if_needed_clean_break branch from f5f3fff to 8016bd2 Compare April 3, 2024 20:02

pllim reviewed Apr 3, 2024

View reviewed changes

docs/changes/16181.other.rst Outdated Show resolved Hide resolved

Update 16181.other.rst

f010f71

Co-authored-by: P. L. Lim <[email protected]>

astrofrog approved these changes Apr 4, 2024

View reviewed changes

astrofrog merged commit 35d18ce into astropy:main Apr 4, 2024

neutrinoceros deleted the api/copy_if_needed_clean_break branch April 4, 2024 10:03

pllim mentioned this pull request Aug 12, 2024

MNT: Numpy 2.1 compatibility for HDF5 table reader #16814

Merged

1 task

Uh oh!

API: align default value for copy arguments with numpy at runtime (copy=False raises an error if copies cannot be avoided) #16181

API: align default value for copy arguments with numpy at runtime (copy=False raises an error if copies cannot be avoided) #16181

Uh oh!

Conversation

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

Uh oh!

Description

Uh oh!

github-actions bot commented Mar 8, 2024

Uh oh!

github-actions bot commented Mar 8, 2024

Uh oh!

neutrinoceros commented Mar 8, 2024

Uh oh!

pllim commented Mar 8, 2024

Uh oh!

pllim commented Mar 8, 2024

Uh oh!

neutrinoceros commented Mar 9, 2024

Uh oh!

taldcroft commented Mar 14, 2024

Uh oh!

neutrinoceros commented Mar 14, 2024

Uh oh!

neutrinoceros commented Mar 14, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Mar 15, 2024

Uh oh!

neutrinoceros commented Mar 15, 2024

neutrinoceros commented Mar 8, 2024 •

edited by pllim

Loading

neutrinoceros Mar 15, 2024 •

edited

Loading

pllim Mar 15, 2024 •

edited

Loading

pllim commented Mar 15, 2024 •

edited

Loading