BUG: fix compatibility with numpy 2.0 for ndarray suclasses overriding ndarray.sort and ndarray.argsort by neutrinoceros · Pull Request #15929 · astropy/astropy

neutrinoceros · 2024-01-23T07:58:09Z

Description

This pull request is to address one of the three failures reported in #15926
~~This is currently based atop #15925~~

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-01-23T07:58:37Z

github-actions · 2024-01-23T07:58:41Z

👋 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?

mhvk · 2024-01-23T15:36:21Z

astropy/units/quantity.py

    # ensure we do not return indices as quantities
-    def argsort(self, axis=-1, kind="quicksort", order=None):
-        return self.view(np.ndarray).argsort(axis=axis, kind=kind, order=order)
+    def argsort(self, axis=-1, kind=None, order=None, *, stable=False):


For previous cases of signature changes, we did

if NUMPY_LT_2_0: def argsort(old-signature): ... else: def argsort(new-signature): ...

I think that is preferable, since it means the signature is correct for the given numpy version, and there is no slowdown.

I guess an alternative would be to just pass *args, **kwargs? Especially given that "quicksort" is the default anyway.

Oh I didn't realise that kind=None implied "quicksort". In that case, might as well 100% stick to numpy's signature.

astropy/utils/masked/core.py

docs/changes/units/15929.bugfix.rst

pllim · 2024-01-24T16:40:00Z

Is this the only one left that would make devdeps green again? 🤞

Thanks for all your help!

neutrinoceros · 2024-01-24T16:44:15Z

Is this the only one left that would make devdeps green again?

Yes and no: it is the only one left so far, but don't get your hopes too high: there are still a couple issues with numpy dev that require we don't have solutions to at the moment.

pllim · 2024-01-24T17:10:12Z

In that case, are you able to rebase and at least fix the argsort stuff in this PR? Less traceback in the log would ease the rest of the fix. Thanks!

neutrinoceros · 2024-01-24T17:12:39Z

rebased !

pllim · 2024-01-24T18:45:25Z

Now I am confused about the CircleCI jobs. The jobs here are green but they ran with pluggy 1.4.0 too. Are the failures not appearing all the time? 🤯

pllim · 2024-01-24T18:47:57Z

But back to the devdeps job, can you please confirm that the remaining 18 failures in https://github.com/astropy/astropy/actions/runs/7643754025/job/20826541495?pr=15929 are unrelated? Thanks!

neutrinoceros · 2024-01-24T18:49:41Z

I don't get it either. My immediate instinct is to assume that pytest itself isn't fully deterministic 🤷🏻‍♂️ ?

neutrinoceros · 2024-01-24T18:59:13Z

can you please confirm that the remaining 18 failures in astropy/astropy/actions/runs/7643754025/job/20826541495?pr=15929 are unrelated?

they are. I have them all on my radar:

TST: (numpy dev) fix return type for a selection of ufuncs applied to Column #15941 solves two of them
the rest of them share a root cause that is probably not trivial to address (which we discussed in passing in BUG: fix compatibility with numpy 2.0 for np.broadcast_arrays (now returns a tuple) #15930)

pllim · 2024-01-24T19:05:02Z

Great! Hopefully @mhvk can review soon though the diff LGTM at a glance. Thanks!

mhvk

@neutrinoceros - still some questions/suggestions for simplification...

mhvk · 2024-01-24T12:39:33Z

astropy/units/quantity.py

+    else:
+
+        def argsort(self, axis=-1, kind=None, order=None, *, stable=False):
+            kwargs = dict(axis=axis, order=order)


Do we actually need to duplicate this check on the arguments? Why are we not just sending everything upstream?

I think you're right, it's not needed now that I've changed the default value for kind ! thanks !

mhvk · 2024-01-24T19:30:08Z

astropy/utils/masked/core.py

+                second, etc.  A single field can be specified as a string, and not
+                all fields need be specified, but unspecified fields will still be
+                used, in dtype order, to break ties.
+            stable: bool, keyword-only


Since it is not actually used, in this case I think we can get away with just defining argsort for all numpy and just not passing on stable. Then, here write

Only for compatibility with np.sort (for numpy >= 2.0). Ignored.

neutrinoceros · 2024-01-24T20:47:38Z

with your suggestions in, the diff is now much smaller. Thanks @mhvk !

mhvk

This is nice! Some small comments, partially just to improve it slightly more (with no change in API, so can still backport).

mhvk · 2024-01-25T16:49:43Z

astropy/utils/masked/core.py

            all fields need be specified, but unspecified fields will still be
            used, in dtype order, to break ties.
+        stable: bool, keyword-only
+            Only for compatibility with np.sort. Ignored.


Maybe phrase just like for kind, i.e.,

stable: bool, keyword-only, ignored Sort stability. Present only to allow subclasses to work.

yeah that's more consistent with how kind is documented !

mhvk · 2024-01-25T16:57:07Z

astropy/utils/masked/core.py

+
+        Notes
+        -----
+        stable is only used with numpy 2.0 and newer, and is ignored otherwise.


Yes, good idea to have a note. But kind is ignored too... How about,

Masked items will be sorted to the end. The implementation is via `np.lexsort` and thus ignores the ``kind`` and ``stable`` arguments; they are present only so that subclasses can pass them on.

astropy/utils/masked/core.py

mhvk · 2024-01-25T17:05:09Z

astropy/utils/masked/core.py

        # TODO: probably possible to do this faster than going through argsort!
-        indices = self.argsort(axis, kind=kind, order=order)
+        argsort_kwargs = dict(kind=kind, order=order)
+        if not NUMPY_LT_2_0:


By setting the default to np._NoValue, we can change this to

if stable is not np._NoValue: argsort_kwargs["stable"] = stable

I do like that you pass them on, since in principle a subclass could override argsort and do something with it.

…g ndarray.sort and ndarray.argsort

mhvk

OK, let's get it in! Thanks, @neutrinoceros!

mhvk · 2024-01-25T18:53:46Z

astropy/utils/masked/core.py

+        Notes
+        -----
+        Masked items will be sorted to the end. The implementation
+        is via `np.lexsort` and thus ignores the ``kind`` and ``stable`` arguments;


Darn, this should have been numpy.lexsort for readthedocs to be happy...

Since it must be getting late in Europe, I took the liberty to fix this directly on this branch. Hope you all don't mind!

Thanks @pllim !

…ndarray suclasses overriding ndarray.sort and ndarray.argsort

pllim · 2024-01-25T19:37:12Z

Down to 2 failures! 🚀

pllim · 2024-01-25T19:40:15Z

Let's try again

@meeseeksdev backport to v6.0.x

lumberbot-app · 2024-01-25T19:40:21Z

Could not push to auto-backport-of-pr-15929-on-v6.0.x due to error, aborting.

pllim · 2024-01-25T19:41:03Z

C'mone bot. Don't be mad at me.

@meeseeksdev backport to v6.0.x

lumberbot-app · 2024-01-25T19:41:09Z

Could not push to auto-backport-of-pr-15929-on-v6.0.x due to error, aborting.

…t_stable BUG: fix compatibility with numpy 2.0 for ndarray suclasses overriding ndarray.sort and ndarray.argsort

Backport PR #15929 on branch v6.0.x (BUG: fix compatibility with numpy 2.0 for ndarray suclasses overriding ndarray.sort and ndarray.argsort)

mhvk · 2024-01-25T21:53:03Z

Thanks, @pllim!

neutrinoceros changed the title ~~TST: fix deprecation warnings from numpy dev's __array_wrap__ signature~~ BUG: fix compatibility with numpy 2.0 for MaskedNDarray.sort Jan 23, 2024

github-actions bot added table nddata units convolution utils.masked labels Jan 23, 2024

neutrinoceros force-pushed the tst/numpy2/ma_sort_stable branch from 627c294 to 7e5678a Compare January 23, 2024 07:59

neutrinoceros mentioned this pull request Jan 23, 2024

TST: (numpy dev) new failing tests #15926

Closed

neutrinoceros force-pushed the tst/numpy2/ma_sort_stable branch from 7e5678a to d55d0cf Compare January 23, 2024 08:57

neutrinoceros changed the title ~~BUG: fix compatibility with numpy 2.0 for MaskedNDarray.sort~~ BUG: fix compatibility with numpy 2.0 for ndarray suclasses overriding ndarray.sort and ndarray.argsort Jan 23, 2024

This was referenced Jan 23, 2024

EXP: combine #15925 #15929 and #15930 #15931

Closed

BUG: fix compatibility with numpy 2.0 for np.broadcast_arrays (now returns a tuple) #15930

Merged

mhvk reviewed Jan 23, 2024

View reviewed changes

astropy/utils/masked/core.py Show resolved Hide resolved

pllim added this to the v6.0.1 milestone Jan 23, 2024

pllim added no-changelog-entry-needed numpy-dev backport-v6.0.x labels Jan 23, 2024

pllim reviewed Jan 23, 2024

View reviewed changes

docs/changes/units/15929.bugfix.rst Outdated Show resolved Hide resolved

neutrinoceros force-pushed the tst/numpy2/ma_sort_stable branch from 93ed82c to 56b8f0f Compare January 24, 2024 06:53

neutrinoceros marked this pull request as ready for review January 24, 2024 07:12

neutrinoceros force-pushed the tst/numpy2/ma_sort_stable branch from 56b8f0f to 42d9b57 Compare January 24, 2024 17:12

pllim mentioned this pull request Jan 24, 2024

Pluggy 1.4.0 now warns about deprecated hook wrapper style matplotlib/pytest-mpl#221

Closed

neutrinoceros mentioned this pull request Jan 24, 2024

TST: (numpy dev) fix return type for a selection of ufuncs applied to Column #15941

Merged

1 task

mhvk reviewed Jan 24, 2024

View reviewed changes

neutrinoceros force-pushed the tst/numpy2/ma_sort_stable branch from 42d9b57 to 930e0dd Compare January 24, 2024 20:46

neutrinoceros force-pushed the tst/numpy2/ma_sort_stable branch from 930e0dd to 5ddb2ae Compare January 24, 2024 21:06

This was referenced Jan 25, 2024

TST: MaskedNDArray.__array_wrap__ is incompatible with numpy dev #15947

Closed

EXP: #15948 + #15941 + #15929 #15949

Closed

mhvk reviewed Jan 25, 2024

View reviewed changes

BUG: fix compatibility with numpy 2.0 for ndarray suclasses overridin…

ef5748c

…g ndarray.sort and ndarray.argsort

neutrinoceros force-pushed the tst/numpy2/ma_sort_stable branch from 5ddb2ae to ef5748c Compare January 25, 2024 17:22

mhvk approved these changes Jan 25, 2024

View reviewed changes

mhvk reviewed Jan 25, 2024

View reviewed changes

Fix Sphinx ref so RTD can render

50470df

pllim merged commit 74bdf27 into astropy:main Jan 25, 2024

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jan 25, 2024

Backport PR astropy#15929: BUG: fix compatibility with numpy 2.0 for …

e14504c

…ndarray suclasses overriding ndarray.sort and ndarray.argsort

meeseeksmachine mentioned this pull request Jan 25, 2024

Backport PR #15929 on branch v6.0.x (BUG: fix compatibility with numpy 2.0 for ndarray suclasses overriding ndarray.sort and ndarray.argsort) #15952

Closed

pllim added a commit to pllim/astropy that referenced this pull request Jan 25, 2024

Merge pull request astropy#15929 from neutrinoceros/tst/numpy2/ma_sor…

41cf80a

…t_stable BUG: fix compatibility with numpy 2.0 for ndarray suclasses overriding ndarray.sort and ndarray.argsort

pllim mentioned this pull request Jan 25, 2024

Backport PR #15929 on branch v6.0.x (BUG: fix compatibility with numpy 2.0 for ndarray suclasses overriding ndarray.sort and ndarray.argsort) #15953

Merged

1 task

pllim added a commit that referenced this pull request Jan 25, 2024

Merge pull request #15953 from pllim/manual-backport-pr15929-v6.0.x

75fbc4f

Backport PR #15929 on branch v6.0.x (BUG: fix compatibility with numpy 2.0 for ndarray suclasses overriding ndarray.sort and ndarray.argsort)

neutrinoceros deleted the tst/numpy2/ma_sort_stable branch January 25, 2024 22:25

Uh oh!

Conversation

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

Uh oh!

Description

Uh oh!

github-actions bot commented Jan 23, 2024

Uh oh!

github-actions bot commented Jan 23, 2024

Uh oh!

mhvk Jan 23, 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!

neutrinoceros Jan 23, 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!

Uh oh!

Uh oh!

pllim commented Jan 24, 2024

Uh oh!

neutrinoceros commented Jan 24, 2024

Uh oh!

pllim commented Jan 24, 2024

Uh oh!

neutrinoceros commented Jan 24, 2024

Uh oh!

pllim commented Jan 24, 2024

Uh oh!

pllim commented Jan 24, 2024

Uh oh!

neutrinoceros commented Jan 24, 2024

Uh oh!

neutrinoceros commented Jan 24, 2024

Uh oh!

pllim commented Jan 24, 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!

neutrinoceros commented Jan 24, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Jan 25, 2024

Uh oh!

pllim commented Jan 25, 2024

Uh oh!

lumberbot-app bot commented Jan 25, 2024

Uh oh!

pllim commented Jan 25, 2024

Uh oh!

lumberbot-app bot commented Jan 25, 2024

Uh oh!

neutrinoceros commented Jan 23, 2024 •

edited

Loading

mhvk Jan 23, 2024 •

edited

Loading

neutrinoceros Jan 23, 2024 •

edited

Loading