return NaN in npy_ObjectMax() and npy_ObjectMin() if it's an argument

[mamikonyan]

This is a patch I posted to #4903, but it must have fallen through the cracks. It brings the code inline with the documentation in that it always returns the NaN argument (first, if both).

diff --git a/numpy/core/src/umath/funcs.inc.src b/numpy/core/src/umath/funcs.inc.src
index 3aad44c..0102a4f 100644
--- a/numpy/core/src/umath/funcs.inc.src
+++ b/numpy/core/src/umath/funcs.inc.src
@@ -65,8 +65,13 @@ npy_Object@Kind@(PyObject *i1, PyObject *i2)
     PyObject *result;
     int cmp;

-    cmp = PyObject_RichCompareBool(i1, i2, @OP@);
-    if (cmp < 0) {
+    if (PyFloat_Check(i1) && Py_IS_NAN(PyFloat_AS_DOUBLE(i1))) {
+        cmp = 1;
+    }
+    else if (PyFloat_Check(i2) && Py_IS_NAN(PyFloat_AS_DOUBLE(i2))) {
+        cmp = 0;
+    }
+    else if ((cmp = PyObject_RichCompareBool(i1, i2, @OP@)) < 0) {
         return NULL;
     }
     if (cmp == 1) {

[charris]

@mamikony It would be best if you made a normal PR out of this.

[seberg]

Yes please, though I think rather then do checks with floats, invert the logic (i.e. use <= instead of >, etc.), which should have the same effect.

[seberg]

Wait, we already removed that old non-richcompare branch. Didn't that already fix this fully?

[mamikonyan]

This is a different issue. Previously, the result was dependent on the relative addresses in case of NaNs because of the builtin cmp() operator, i.e., code internal to the Python interpreter. Now, the result is always the second argument when either is NaN because of the logic of the functions in question.

>>> np.minimum(np.array(float('nan'), object), 1)
1
>>> np.minimum(1, np.array(float('nan'), object))
nan

[mamikonyan]

As @charris pointed out in #4903, the current behavior is inconsistent with the Python builtins min()/max(), which will return the first argument, in this case. This is also inconsistent with NumPy documentation, which described what seems to me to be far more logical behavior: return the first NaN.

[seberg]

Hmmmpf, I don't like this. I admit since people should implement __float__ for their own objects, it should work quite universally (not sure if the PyFloat_Check is correct and I am sure it needs some error handling, though), but complex is still a problem. I guess there is no reasonable way to do it differently? This is a rather universal problem for object typed arrays, the fact that python screws it up, too, somewhat hints at that. NaN has some weird properties and I don't know of any universal simple way to check for it.

Anyway, I don't know what the right solution is. There are similar issues like this all over, maybe we should do this, because it is the best we have, or maybe we should do a second comparison to find NaNs weirdness. Or maybe it is all slow/useless and the user needs to be careful.

[mamikonyan]

@seberg You make a couple of points to consider. First, what error checking do you have in mind? Second, complex numbers numbers aren't ordered like the reals, so both, Python and NumPy, correctly issue the exception
TypeError: no ordering relation is defined for complex numbers
Finally, what properties of NaNs bother you. I think this is the reason we return the actual argument we received, instead of a new (possibly different kind of) NaN.

[seberg]

OK, so complex may not b e a problem here. What I meant with error checking is, that checking for a python float or subtype will not find for example np.float32, so you probably really would have to try to get the float value and that might fail.
What bothers me is that I don't see a genera way to find things like decimal.decimal('NaN') and np.float32(np.nan) and complex NaNs all in one comprehensive way.

[mamikonyan]

I understand, but I don't think we can do better than Python here because we ultimately dispatch to it. In the case of Decimal, apparently it doesn't like NaNs at all:
min(decimal.Decimal('nan'), 1) yields decimal.InvalidOperation: comparison involving NaN