iframe-proxy

MaanasArora · 2025-12-11T05:01:00Z

Follows #29737 and #30328 (and supersedes #29987) to implement a sort_compare slot that can be used to register sorting methods using a comparison function. This functionality also existed in the ArrFuncs slots, but now we implement the new-style sorting methods, and the array method context is passed to the comparison function rather than the array itself.

I've tested this with mpfdtype on the user dtypes repo (though having a bit of an issue with installation... the slot wasn't visible there, so had to redefine just the macro). Unfortunately there don't seem to be dtypes we can write tests for within numpy itself, though I did a small experiment with the scaled float dtype. ping @seberg @mhvk @ngoldbaum if you're interested - thanks!

ngoldbaum

@mhvk and @charris I think had strong opinions about prior versions of this. I'd appreciate it if they could weigh in on the approach.

Overall I think this is a nice cleanup and since StringDType wanted it, I wouldn't be surprised if future DTypes do as well.

ngoldbaum · 2025-12-12T15:27:08Z

+    If defined, implements a comparison function for two array elements
+    for use in sorting and argsorting. This can be defined in place of the
+    custom sort functions using the ArrayMethod API (see :ref:`array-methods-sorting`)
+    to implement sorting for the DType.


maybe add a mention for why you'd want to do this - e.g. if comparisons have some big cost that can be amortized over the whole array

I've added a note - a bit on the detailed side, hopefully that works, thanks!

MaanasArora · 2025-12-12T23:59:34Z

Thanks for looking @ngoldbaum!

Overall I think this is a nice cleanup and since StringDType wanted it, I wouldn't be surprised if future DTypes do as well.

Agreed, the full loop implementation would probably be overkill in many cases. I think this would be necessary to expose the sorts on the user-dtypes too since they don't define full sorts now, so a good way to test the new infrastructure!

…comparison function

…are`

MaanasArora · 2026-03-16T12:58:27Z

Added release note and support for GIL flag. @seberg Friendly ping if you want to look at this when you get the chance!

mhvk

Apologies for the delay in looking at this - it really slipped too far....

This looks mostly great, some small comments in-line.

A bigger question is about the name: should it just be compare? I.e., can it not be used also for dtype comparison more generally, replacing NPY_DT_PyArray_ArrFuncs_compare? I cannot really think what the difference would be... (but if we go that route, we can adjust comparison in a follow-up PR). Let me ping @seberg on this bit...

Relatedly, I think not if we want to get rid of ArrFuncs eventually, but let me ask just in case: would it make sense to by default use NPY_DT_PyArray_ArrFuncs_compare if it is available?

mhvk · 2026-03-17T12:06:03Z

+            .nout = 1,
+            .dtypes = sort_dtypes,
+            .slots = sort_slots,
+            .flags = NPY_METH_NO_FLOATINGPOINT_ERRORS,


Given it being set here, is it still necessary to also do it in npy_default_get_(arg)sort_loop?

Ah thanks, good catch! It's not - I'll remove it from there.

It's correct in the get_loop, here it is rather irrelevant (it's a default, but for the default get_loop)

Not sure I understand sorry - don't they have the same effect? But I can remove it from here if it's a design / reuse thing.

You can remove it here, it doesn't matter really. The value here is what get_loop uses if you don't provide a custom get_loop. But you do, so it is effectively not used.

mhvk · 2026-03-17T12:06:26Z

+            .flags = NPY_METH_NO_FLOATINGPOINT_ERRORS,
+    };
+    if (dtype->flags & NPY_NEEDS_PYAPI) {
+        sort_spec.flags |= NPY_METH_REQUIRES_PYAPI;


Same question here.

Also, in the other places, it uses PyDataType_REFCHK - any reason for the difference?

Same here, it's not!

I think PyDataType_REFCHK is used for descriptors and not the dtypemeta (at least type wise).

Yeah, NPY_NEEDS_PYAPI flag makes more sense. REFCHK doesn't matter, so should be changed in the first one.

seberg · 2026-03-17T13:00:33Z

A bigger question is about the name: should it just be compare?

Maybe it's best to keep it this way? The context is based on the sorting, I think it is fine. We could still add a mechanism to return a mechanism for both, but I am tempted to say it's fine to split it. In the end, comparison needs both a superset (<= rather than just <) and a subset (no worries about NaN order)...

I guess this is partly saying that defining the comparisons ufuncs is just easy enough, while sorting is a bit tedious to implement.

seberg

This looks good, sorry should have pushed this earlier. I am OK with seeing this is a bit distinct to comparisons.

Mainly, because the alternative to me would seem to be having operands for all normal comparisons + variations for NaN order for less and greater.
But... that adds a complexity that I am not sure is all that helpful (because as mentioned adding the ufunc family is pretty straight forward).

Would it be easy to use this for the SFloat (without losing the other test coverage completely since I think string ufuncs use that also?)

seberg · 2026-03-17T13:03:58Z

+    PyArrayMethod_SortParameters *parameters = (PyArrayMethod_SortParameters *)context->parameters;
+    *flags |= NPY_METH_NO_FLOATINGPOINT_ERRORS;
+
+    if (PyDataType_REFCHK(context->descriptors[0])) {


As mentioned below, should use the NPY_REQUIRES_API flag here. Otherwise should be OK, if someone wants to relax this, they'll just have to implement the whole thing...

This (and removing from registration) is done, thanks! (I'll push sfloat changes later)

seberg · 2026-03-17T13:06:15Z

+            .nout = 1,
+            .dtypes = sort_dtypes,
+            .slots = sort_slots,
+            .flags = NPY_METH_NO_FLOATINGPOINT_ERRORS,


You can remove it here, it doesn't matter really. The value here is what get_loop uses if you don't provide a custom get_loop. But you do, so it is effectively not used.

seberg · 2026-03-17T13:07:12Z

 } PyArrayMethod_SortParameters;

+typedef int (PyArrayDTypeMeta_SortCompare)(const void *a, const void *b,
+        PyArrayMethod_Context *context);


Might even be better this way, but I am slightly partial towards putting the context first, just because it is the way in ufunc loops.

Unfortunately I cast directly to CompareFunc, so seems hard to do?

What if we make a wrapper for the old-style CompareFunc? For the numpy dtypes we could (in a follow-up PR perhaps...) then write explicit loops, in order to keep the same performance.

MaanasArora · 2026-03-17T13:13:31Z

No worries at all, thanks for reviewing @mhvk @seberg!

A bigger question is about the name: should it just be compare?

Yes, as Sebastian said, if we use this for comparison, I think we lose some functionality - also, keeping this tied to sorting makes it easier to separate concerns if a dtype has some sort-specific quirk (like some optimization that can be made for sort but not compare due to the superset/subset distinction). If we do add a general compare, it can probably be used to fill this by default, but that's for the future!

One note though, this should also probably be used for binsearch / searchsorted (if it is revamped), as that is similar enough.

Relatedly, I think not if we want to get rid of ArrFuncs eventually, but let me ask just in case: would it make sense to by default use NPY_DT_PyArray_ArrFuncs_compare if it is available?

The context is sort-specific - though it can probably be filled in if we wrap! Aside from that hurting deprecation (I think?) one concern with that might be blurring legacy code i.e. the existing dtypes would suddenly define these array methods (which might be nice, but I didn't go that way yet - though it probably needs to be done one way or the other anyway!)

MaanasArora · 2026-03-17T13:20:36Z

Sorry, missed the top-level comment - thanks for reviewing.

Would it be easy to use this for the SFloat (without losing the other test coverage completely since I think string ufuncs use that also?)

Yes I think it's quite easy, in fact IIRC I used it for testing - going to push that!

…check

mhvk · 2026-03-17T13:43:18Z

Now saw your reply to my bigger comment. I guess I can see that comparison might be different for sort that generally, but really it seems hard to see how that would be the case where one only compares individual elements, not a whole set of them. And for a whole set, one probably just wants to register one's own sort loop.

Overall, it seems fine for comparison to remain as simple as possible and be used wherever (as it is for the old-style function, I think...).

mhvk · 2026-03-17T13:46:33Z

Just trying to keep playing devil's advocate a bit: the NaN case is indeed one where regular comparisons may different from what is needed for sorting, but wouldn't that be covered by having a context passed in?

seberg · 2026-03-17T14:06:44Z

but really it seems hard to see how that would be the case where one only compares individual elements, not a whole set of them. And for a whole set, one probably just wants to register one's own sort loop.

Well, they are slightly different if you look ahead:

Normal comparisons: <, >, <=, >=, ==, !=. (I.e. enumerated by Python richcmp also)
Sort comparisons: < + NaN considered largest value, > + NaN considered smallest value
- Right now, we also have the opposite NaN ordering, although the sort-loop could take care of that as well.
- We also don't need the stable information here, I think.

I am not opposed at all to trying to merge. The big thing is that we definitely need to change the current sorting implementations (which seems fine) and either keep the old one, or add a mini-wrapper (and live with the slight performance hit).

Changing+wrapping/duplicating the generic sort loop is fine. I think the main thing is that context really needs to be different. If just int compare(PyArray_Descr *descr, void *left, void *right, CMP_ENUM cmp) function, or even a get_compare() to allow things like locking/unlocking (for string dtype) and passing in NPY_AUXDATA/specialization.

MaanasArora · 2026-03-17T22:55:21Z

github-actions Bot added the 01 - Enhancement label Dec 11, 2025

ngoldbaum reviewed Dec 12, 2025

View reviewed changes

MaanasArora mentioned this pull request Jan 16, 2026

FEAT: Implementing NPY_DT_PyArray_ArrFuncs_compare numpy/numpy-quaddtype#48

Merged

MaanasArora added 3 commits March 16, 2026 08:40

ENH: Add sort_compare dtype slot to register new-style sorts using …

4620c76

…comparison function

ENH: Dynamic memory allocation for sort method names using `sort_comp…

7ac9b2e

…are`

DOC: Add note for when to define full sorting array methods

0d8c026

MaanasArora force-pushed the enh/sort-compare branch from e4de2bd to 0d8c026 Compare March 16, 2026 12:41

MaanasArora added 2 commits March 16, 2026 08:55

ENH: Add support for PyAPI flag in sort compare

30755b2

Add release note

7207205

mhvk reviewed Mar 17, 2026

View reviewed changes

seberg reviewed Mar 17, 2026

View reviewed changes

ENH: Update default sort method flags to use NPY_NEEDS_PYAPI for GIL …

e3f3640

…check

mhvk mentioned this pull request Apr 21, 2026

ENH: ArrayMethod sorting for built-in dtypes #31278

Closed

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Uh oh!

Conversation

MaanasArora commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum left a comment • 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!

MaanasArora commented Dec 12, 2025

Uh oh!

MaanasArora commented Mar 16, 2026

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!

seberg commented Mar 17, 2026

Uh oh!

seberg 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!

MaanasArora commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaanasArora commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Mar 17, 2026

Uh oh!

mhvk commented Mar 17, 2026

Uh oh!

seberg commented Mar 17, 2026

Uh oh!

MaanasArora commented Mar 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MaanasArora commented Dec 11, 2025 •

edited

Loading

ngoldbaum left a comment •

edited

Loading

MaanasArora commented Mar 17, 2026 •

edited

Loading

MaanasArora commented Mar 17, 2026 •

edited

Loading