ENH: show warning when np.maximum receives more than 2 inputs #29052

lvllvl · 2025-05-25T23:41:15Z

attempts to resolve #27639

numpy/_typing/_ufunc.pyi

seberg

I wouldn't mind discussing whether we should start forcing out= more generally, since the overhead of using out= is pretty low now.

But maybe we say maximum is special, which is fine:

You are doing twice the same thing in the same code.
Do not just go to an error, this should go through a DeprecationWarning. Maybe one can argue that it is almost always a bug and we go directly to a VisibleDeprecationWarning. But I am a bit skeptical, also because DeprecationWarnings are seen more these days anyway.

numpy/_core/tests/test_umath.py

numpy/_core/src/umath/ufunc_object.c

mhvk

I don't think this should happen just for maximum (though that discussion is perhaps best done in #27639).

But it certainly shouldn't be in the hot path for every ufunc -- instead, on l.4321 (original one), one could do

if NPY_UNLIKELY(len_args != nin) {
    if (len_args <= nop) {
       /* new deprecation stuff */
    }
    else {
       PyErrFormat(...);
       goto fail;
    } 
}

seberg

Thanks, but this needs some work, also you didn't address the discussion around minimum and maximum.

numpy/_core/src/umath/ufunc_object.c

seberg · 2025-06-02T09:08:18Z

numpy/_core/src/umath/ufunc_object.c

+                "is deprecated; use out=keyword or np.maximum.reduce.");
+            PyErr_Format(PyExc_TypeError,
+                "np.maximum() takes exactly 2 input arguments (%zd given).",
+                len_args);


It doesn't make sense to warn and error. You should just give the warning and test it. A warning can be turned into an error, in which case yuo need extra logic.

numpy/_core/src/umath/ufunc_object.c

lvllvl · 2025-08-03T22:40:56Z

Thanks, but this needs some work, also you didn't address the discussion around minimum and maximum.

Hi @seberg,

I added some changes related to the discussion around maximum/minimum, e.g.,

when 2 or more positional args are pass to np.maximum, np.minimum there will be a DeprecationWarning
the warning will be about the positional out, the behavior will otherwise be unchanged

Let me know if that addresses all your concerns. I can add other changes as well. I don't know if changing documentation would be in this scope but I can add that as well, if needed.

seberg

I suppose @mhvk was also mildly in favor of just doing this (limited to minimum/maximum), so this looks pretty close, thanks.

numpy/_core/src/umath/umathmodule.c

seberg · 2025-08-05T08:48:42Z

numpy/_core/tests/test_umath.py

+        a = np.array([1])
+        b = np.array([2])
+        out = np.empty_like(a)
+        with pytest.warns(DeprecationWarning, match=r"Passing more than 2 positional arguments to np\.maximum is deprecated"):


This is failing the linter for good reason.

seberg · 2025-08-05T08:53:47Z

numpy/_core/src/umath/ufunc_object.c

@@ -4363,6 +4363,20 @@ ufunc_generic_fastcall(PyUFuncObject *ufunc,
            Py_INCREF(tmp);
            PyTuple_SET_ITEM(full_args.out, i-nin, tmp);
        }
+
+        /* Extra positional args but *no* keywords */
+        if ((ufunc == UFUNC_MAXIMUM || ufunc == UFUNC_MINIMUM) && !all_none) {


This is a bit tautological, you already have both ufunc == checks just below. TBH, I think it would be fine to just make one path though and don't care about the duplication. (or use ufunc->name.)

seberg · 2025-08-05T08:54:21Z

numpy/_core/tests/test_umath.py

+        out = np.empty_like(a)
+
+        with pytest.warns(DeprecationWarning, match=r"Passing more than 2 positional arguments to np\.minimum is deprecated"):
+            result = np.minimum(a, b, out)


Sorry for not flagging earlier, but please move this test (following the pattern there) to test_deprecations.py.

In general I don't care too much about it, but I do care about testing the error path when the warning is raised as an error, and that is not tested here, but the pattern in the file always tests it.

(And when you do that, you should see that the C code is missing error checks and the test fails in weird ways.)

seberg · 2025-08-06T06:51:48Z

numpy/_core/src/umath/ufunc_object.c

+
+        /* Extra positional args but *no* keywords */
+        /* DEPRECATED NumPy 2.4, 2025-08 */
+        if (strcmp(ufunc->name, "maximum") == 0 || strcmp(ufunc->name, "minimum") == 0) {


FWIW, I actually liked the old version of using the object, but maybe it doesn't matter on this path anyway.

Please don't use snprintf, DEPRECATE is a useful macro, but you don't need to use it, you can use PyWarn_Format (or whatever it was).

Agreed that the direct, very fast check with the object was better. We're not in a really hot path here, but still nice if something like np.add(in1, in2, out) remains as fast as possible.

Ok, my mistake I should've clarified before changing back to strcmp. I can change it back if you'd like.

I simplified the if-conditional like @mhvk suggested and I think I'll stick with the DEPRECATE macro, if you don't mind.

EDIT: I actually just re-inserted the faster version, instead of the strcmp version.

numpy/_core/src/umath/ufunc_object.c

mhvk

Agreed that this is mostly done - just small ideas for improvements.

mhvk · 2025-08-06T19:58:59Z

numpy/_core/src/umath/ufunc_object.c

+
+        /* Extra positional args but *no* keywords */
+        /* DEPRECATED NumPy 2.4, 2025-08 */
+        if (strcmp(ufunc->name, "maximum") == 0 || strcmp(ufunc->name, "minimum") == 0) {


Agreed that the direct, very fast check with the object was better. We're not in a really hot path here, but still nice if something like np.add(in1, in2, out) remains as fast as possible.

mhvk · 2025-08-06T19:59:41Z

numpy/_core/tests/test_deprecations.py

@@ -480,3 +480,27 @@ def test_deprecated(self):
    def test_not_deprecated(self, align):
        # if the user passes a bool, it is accepted.
        self.assert_not_deprecated(lambda: np.dtype("f8", align=align))
+
+class TestTooManyArgsMaximum(_DeprecationTestCase):


This needs an extra emptyline for PEP8 (below too).

Actually, if the message becomes the same anyway, you can combine the two with

class TestTooManyArgsExtremum(_DeprecationTestCase): ... @pytest.mark.parametrize(ufunc, [np.minimum, np.maximum]) def test_extremem_3_args(self, ufunc): self.assert_deprecated(ufunc, args=(np.ones(1), np.zeros(1), np.empty(1)))

Ok thanks for the feedback. I made changes to the test and used Ruff to format the test.

mhvk · 2025-08-06T20:10:34Z

numpy/_core/src/umath/ufunc_object.c

+        if (strcmp(ufunc->name, "maximum") == 0 || strcmp(ufunc->name, "minimum") == 0) {
+            const char *which = ufunc->name; // "maximum" or "minimum"
+            char msg[256];
+            snprintf(msg, sizeof(msg),


For the deprecation warning, I would suggest not worrying about adapting it to the name, but just keep it simple. Also, it should make clear why it is being deprecated. Maybe,

"Passing more than 2 positional arguments to np.maximum " "and np.minimum'is deprecated. If you meant to use the third " "argument as an output, use the 'out' keyword argument instead. " "If you hoped to work with more than 2 inputs, combine them " "into a single array and get the extrema for the relevant axis."

lvllvl · 2025-08-18T12:36:58Z

Hi @seberg , just wanted to double check - any other changes needed here or is this good to go?

seberg

Sorry, it seems we did forget one thing: A deprecation should probably have a very brief release note.

Everything else looks fine though, just prefer less empty lines :).

seberg · 2025-08-18T14:19:00Z

numpy/_core/src/umath/ufunc_object.c

@@ -65,6 +65,7 @@
 #include "mapping.h"
 #include "npy_static_data.h"
 #include "multiarraymodule.h"
+#include "../multiarray/number.h"


Suggested change

#include "../multiarray/number.h"

#include "number.h"

I think it should work and is what we mostly use.
I don't hate this, but don't want to break the pattern here (e.g. mapping.h is at the same spot).

ok sounds good. committing your suggestion

numpy/_core/src/umath/ufunc_object.c

seberg · 2025-08-18T14:21:26Z

numpy/_core/src/umath/ufunc_object.c

+            if (DEPRECATE(
+                "Passing more than 2 positional arguments to np.maximum and np.minimum "
+                "is deprecated. If you meant to use the third argument as an output, " 
+                "use the 'out' keyword argument instead. If you hoped to work with "


Suggested change

"use the 'out' keyword argument instead. If you hoped to work with "

"use the `out=` keyword argument instead. If you hoped to work with "

numpy/_core/src/umath/ufunc_object.c

seberg · 2025-08-18T14:22:48Z

numpy/_core/tests/test_deprecations.py

+
+    @pytest.mark.parametrize("ufunc", [np.minimum, np.maximum])
+    def test_extremem_3_args(self, ufunc):
+


Suggested change

numpy/_core/tests/test_deprecations.py

lvllvl · 2025-08-19T01:22:37Z

Sorry, it seems we did forget one thing: A deprecation should probably have a very brief release note.

Everything else looks fine though, just prefer less empty lines :).

ok adding an rst file with a short note on the deprecation.

doc/release/upcoming_changes/29052.deprecation.rst

seberg · 2025-08-20T08:37:41Z

Let's give this a shot, thanks @lvllvl.

github-actions bot added the 01 - Enhancement label May 25, 2025

lvllvl force-pushed the issue-27639 branch from cb16c42 to b17c36d Compare May 25, 2025 23:46

jorenham reviewed May 26, 2025

View reviewed changes

numpy/_typing/_ufunc.pyi Outdated Show resolved Hide resolved

lvllvl force-pushed the issue-27639 branch from b17c36d to db2cd35 Compare May 26, 2025 01:21

seberg requested changes May 26, 2025

View reviewed changes

numpy/_core/tests/test_umath.py Outdated Show resolved Hide resolved

numpy/_core/src/umath/ufunc_object.c Outdated Show resolved Hide resolved

mhvk reviewed May 26, 2025

View reviewed changes

lvllvl force-pushed the issue-27639 branch 6 times, most recently from e4d6fa4 to 696f11a Compare May 30, 2025 01:32

lvllvl marked this pull request as ready for review May 31, 2025 19:03

lvllvl requested a review from seberg May 31, 2025 19:04

seberg requested changes Jun 2, 2025

View reviewed changes

lvllvl requested a review from seberg August 4, 2025 16:59

seberg reviewed Aug 5, 2025

View reviewed changes

lvllvl force-pushed the issue-27639 branch from 2e0bbe5 to edd5ce5 Compare August 5, 2025 19:32

seberg reviewed Aug 6, 2025

View reviewed changes

mhvk reviewed Aug 6, 2025

View reviewed changes

lvllvl force-pushed the issue-27639 branch from f40d318 to 64b5553 Compare August 8, 2025 16:50

lvllvl requested a review from seberg August 11, 2025 17:21

seberg reviewed Aug 18, 2025

View reviewed changes

lvllvl added 4 commits August 19, 2025 01:31

ENH: updates based on feedback

65fdb0a

ENH: more changes based on feedback

d787e39

ENH: updating the error checks

5d596dc

ENH: simplify tests, remove snprintf/PyWarn_Format

6fce1fb

lvllvl added 4 commits August 19, 2025 01:31

ENH: re-insert faster comparison

353a39d

ENH: use n_ops struct for comparison

1d98112

DEP: resolve conflicts

abd842f

DEP: adding release note / updates based on feedback

964a9b3

lvllvl force-pushed the issue-27639 branch from 846fd33 to 964a9b3 Compare August 19, 2025 01:45

seberg reviewed Aug 19, 2025

View reviewed changes

doc/release/upcoming_changes/29052.deprecation.rst Outdated Show resolved Hide resolved

lvllvl added 2 commits August 19, 2025 23:07

DEP: formatting changes on release note

09f06da

DEP: formatting changes on release note2

2f86527

seberg approved these changes Aug 20, 2025

View reviewed changes

seberg merged commit 5ba8c96 into numpy:main Aug 20, 2025
77 checks passed

lvllvl deleted the issue-27639 branch August 20, 2025 23:35

	"use the 'out' keyword argument instead. If you hoped to work with "
	"use the `out=` keyword argument instead. If you hoped to work with "


		@pytest.mark.parametrize("ufunc", [np.minimum, np.maximum])
		def test_extremem_3_args(self, ufunc):

Uh oh!

ENH: show warning when np.maximum receives more than 2 inputs #29052

ENH: show warning when np.maximum receives more than 2 inputs #29052

Conversation

lvllvl commented May 25, 2025

Uh oh!

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lvllvl commented Aug 3, 2025

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

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!

lvllvl Aug 7, 2025 • 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!

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!

lvllvl commented Aug 18, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lvllvl commented Aug 19, 2025

Uh oh!

Uh oh!

seberg commented Aug 20, 2025

Uh oh!

Uh oh!

Uh oh!

lvllvl Aug 7, 2025 •

edited

Loading