Skip to content

MAINT, PERF: replace consecutive min and max calls with np.minmax - #32577

Open
ikrommyd wants to merge 3 commits into
numpy:mainfrom
ikrommyd:use-minmax
Open

ikrommyd wants to merge 3 commits into
numpy:mainfrom
ikrommyd:use-minmax

Conversation

@ikrommyd

@ikrommyd ikrommyd commented Sep 10, 2026 •

Copy link
Copy Markdown
Member

PR summary

Since #32231 has been merged, we replace consecutive min and max calls with the new np.minmax. This is a refactoring PR and functionality does not change. This also gives the performance benefit of doing a single pass in those cases.

AI Disclosure

An AI model has been used to conduct an AST search for such location in the source code to make sure I did not miss any by eye.

@ikrommyd

ikrommyd commented Sep 10, 2026 •

Copy link
Copy Markdown
Member Author

There is one more call site that could be replaced in C in PyArray_Ptp but I don't think we really wanna do that cause it would mean a new PyArray_MinMax function that is not part of the public API and a little bit of new C custom C code.
it would need a new minimummaximum field in the NPY_N_OPS_FIELDS module-state struct plus a hand-rolled 2-output reduce, since PyArray_GenericReduceFunction only takes a single out.
PyArray_Ptp has zero internal callers so not worth it IMO.

Signed-off-by: Iason Krommydas <[email protected]>
@ikrommyd ikrommyd changed the title MAINT: replace consecutive min and max calls with np.minmax MAINT, PERF: replace consecutive min and max calls with np.minmax Sep 10, 2026
@ngoldbaum

Copy link
Copy Markdown
Member

PyArray_Ptp has zero internal callers so not worth it IMO

So just to verify, the Python np.ptp function uses the fast path, right?

@ikrommyd

ikrommyd commented Sep 10, 2026 •

Copy link
Copy Markdown
Member Author

PyArray_Ptp has zero internal callers so not worth it IMO

So just to verify, the Python np.ptp function uses the fast path, right?

Yes and I’ve edited it here. It’s one of those functions that call a method on the ndarray that is implemented in python

Comment thread numpy/_core/_methods.py Outdated
Signed-off-by: Iason Krommydas <[email protected]>
Comment thread numpy/_core/_methods.py
min_value, max_value = umr_minimummaximum(
a, axis, None, (None, out), keepdims
)
except TypeError:

@ikrommyd ikrommyd Sep 10, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ngoldbaum I'm pretty sure we have to use TypeError here and not _UFuncNoLoopError only because here we also want to catch __array_ufunc__ of ndarray subclasses rejecting minmax and we want to route that to the fallback. In np.minmax we didn't because the argument was "just use np.min/max separately manually if your __array_ufunc__ doesn't support it" but here it's inside another function that the user has no control over so we have to steer it to the fallback ourselves.

@ikrommyd

Copy link
Copy Markdown
Member Author

Writing it here for visibility, there are a few edge cases regarding legacy user dtypes and array ufunc that we need to take care of here before proceeding with this. Will bring it up in the next numpy meeting.

@jakirkham

Copy link
Copy Markdown
Contributor

Thanks Iason for all of your work on implementing and using minmax! 🙏

Would it make sense to optimize this one as well?

obj1 = PyArray_Max(arr, axis, out);
if (obj1 == NULL) {
goto fail;
}
obj2 = PyArray_Min(arr, axis, NULL);
if (obj2 == NULL) {
goto fail;
}

@ikrommyd

Copy link
Copy Markdown
Member Author

Thanks @jakirkham . I wrote about this place earlier actually in #32577 (comment)
It’s just not as trivial to do and I don’t know if it’s valuable. Do you see a reason? It would only affect someone using this c api function directly. We do not call it in numpy.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants