8000 BUG: maximum, minimum no longer emit warnings on NAN by mattip · Pull Request #12236 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: maximum, minimum no longer emit warnings on NAN #12236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 29, 2018
Merged

BUG: maximum, minimum no longer emit warnings on NAN #12236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da3d162
ENH: maximum, minimum no longer emit warnings on NAN
mattip Oct 21, 2018
24c9faa
MAINT: rearrange logic to not set FP invalid register where possible
mattip Oct 25, 2018
b8d9543
DOC: revise release note
mattip Oct 26, 2018
46452b3
BUG: always clear floatstatus_invalid in max, min
mattip Oct 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions doc/release/1.16.0-notes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,12 +266,13 @@ if ``np.positive(array)`` raises a ``TypeError``. For ``ndarray``
subclasses that override the default ``__array_ufunc__`` implementation,
the ``TypeError`` is passed on.

``maximum`` and ``minimum`` set invalid float status for more dtypes
--------------------------------------------------------------------
Previously only ``float32`` and ``float64`` set invalid float status (by
default emitting a `RuntimeWarning`) when a Nan is encountered in
`numpy.maximum` and `numpy.minimum`. Now ``float16``, ``complex64``,
``complex128`` and ``complex256`` will do so as well.
``maximum`` and ``minimum`` no longer emit warnings
---------------------------------------------------
As part of code introduced in 1.10, ``float32`` and ``float64`` set invalid
float status when a Nan is encountered in `numpy.maximum` and `numpy.minimum`,
when using SSE2 semantics. This caused a `RuntimeWarning` to sometimes be
emitted. In 1.15 we fixed the inconsistencies which caused the warnings to
become more conspicuous. Now no warnings will be emitted.

Umath and multiarray c-extension modules merged into a single module
--------------------------------------------------------------------
Expand Down
35 changes: 13 additions & 22 deletions numpy/core/src/umath/loops.c.src
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1833,10 +1833,7 @@ NPY_NO_EXPORT void
if (!run_unary_reduce_simd_@kind@_@TYPE@(args, dimensions, steps)) {
BINARY_REDUCE_LOOP(@type@) {
const @type@ in2 = *(@type@ *)ip2;
io1 = (io1 @OP@ in2 || npy_isnan(io1)) ? io1 : in2;
}
if (npy_isnan(io1)) {
npy_set_floatstatus_invalid();
io1 = (npy_isnan(io1) || io1 @OP@ in2) ? io1 : in2;
}
*((@type@ *)iop1) = io1;
}
Expand All @@ -1845,13 +1842,11 @@ NPY_NO_EXPORT void
BINARY_LOOP {
@type@ in1 = *(@type@ *)ip1;
const @type@ in2 = *(@type@ *)ip2;
in1 = (in1 @OP@ in2 || npy_isnan(in1)) ? in1 : in2;
if (npy_isnan(in1)) {
npy_set_floatstatus_invalid();
}
in1 = (npy_isnan(in1) || in1 @OP@ in2) ? in1 : in2;
*((@type@ *)op1) = in1;
}
}
npy_clear_floatstatus_barrier((char*)dimensions);
}
/**end repeat1**/

Expand All @@ -1866,15 +1861,15 @@ NPY_NO_EXPORT void
if (IS_BINARY_REDUCE) {
BINARY_REDUCE_LOOP(@type@) {
const @type@ in2 = *(@type@ *)ip2;
io1 = (io1 @OP@ in2 || npy_isnan(in2)) ? io1 : in2;
io1 = (npy_isnan(in2) || io1 @OP@ in2) ? io1 : in2;
}
*((@type@ *)iop1) = io1;
}
else {
BINARY_LOOP {
const @type@ in1 = *(@type@ *)ip1;
const @type@ in2 = *(@type@ *)ip2;
*((@type@ *)op1) = (in1 @OP@ in2 || npy_isnan(in2)) ? in1 : in2;
*((@type@ *)op1) = (npy_isnan(in2) || in1 @OP@ in2) ? in1 : in2;
}
}
npy_clear_floatstatus_barrier((char*)dimensions);
Expand Down Expand Up @@ -2195,14 +2190,11 @@ HALF_@kind@(char **args, npy_intp *dimensions, npy_intp *steps, void *NPY_UNUSED
{
/* */
BINARY_LOOP {
npy_half in1 = *(npy_half *)ip1;
const npy_half in1 = *(npy_half *)ip1;
const npy_half in2 = *(npy_half *)ip2;
in1 = (@OP@(in1, in2) || npy_half_isnan(in1)) ? in1 : in2;
if (npy_half_isnan(in1)) {
npy_set_floatstatus_invalid();
}
*((npy_half *)op1) = in1;
*((npy_half *)op1) = (@OP@(in1, in2) || npy_half_isnan(in1)) ? in1 : in2;
}
/* npy_half_isnan will never set floatstatus_invalid, so do not clear */
Copy link
Member

Choose a reason for hiding this comment

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

when do we set floatstatus_invalid? is that set at the C level?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the non-SSE code sets it in a > b comparisons. which is why I need to clear it in the float-type loops at line 1849, and the complex-type loops at line 2763,

Copy link
Member

Choose a reason for hiding this comment

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

Well, set at the FPU level, not at the C level.

Can't we just reorder the comparisons to do the nan checks first, rather than messing with the FPU flags?

Copy link
Member Author

Choose a reason for hiding this comment

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

trying, let's see what CI finds

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out simd code also sets it, in a way that it is impossible to avoid the clear after the loop finishes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rearranging the order still sets the flag since only one value is checked. It seems cheaper to me to clear the flag than to check both values. In changeset 46452b3 I added back the call to clear the flags, now tests are passing.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think instruction ordering is safe with modern compilers, at least without some work to make the order stick.

}
/**end repeat**/

Expand All @@ -2219,7 +2211,7 @@ HALF_@kind@(char **args, npy_intp *dimensions, npy_intp *steps, void *NPY_UNUSED
const npy_half in2 = *(npy_half *)ip2;
*((npy_half *)op1) = (@OP@(in1, in2) || npy_half_isnan(in2)) ? in1 : in2;
}
npy_clear_floatstatus_barrier((char*)dimensions);
/* npy_half_isnan will never set floatstatus_invalid, so do not clear */
}
/**end repeat**/

Expand Down Expand Up @@ -2761,16 +2753,14 @@ NPY_NO_EXPORT void
@ftype@ in1i = ((@ftype@ *)ip1)[1];
const @ftype@ in2r = ((@ftype@ *)ip2)[0];
const @ftype@ in2i = ((@ftype@ *)ip2)[1];
if ( !(@OP@(in1r, in1i, in2r, in2i) || npy_isnan(in1r) || npy_isnan(in1i))) {
if ( !(npy_isnan(in1r) || npy_isnan(in1i) || @OP@(in1r, in1i, in2r, in2i))) {
in1r = in2r;
in1i = in2i;
}
if (npy_isnan(in1r) || npy_isnan(in1i)) {
npy_set_floatstatus_invalid();
}
((@ftype@ *)op1)[0] = in1r;
((@ftype@ *)op1)[1] = in1i;
}
npy_clear_floatstatus_barrier((char*)dimensions);
}
/**end repeat1**/

Expand All @@ -2786,7 +2776,7 @@ NPY_NO_EXPORT void
const @ftype@ in1i = ((@ftype@ *)ip1)[1];
const @ftype@ in2r = ((@ftype@ *)ip2)[0];
const @ftype@ in2i = ((@ftype@ *)ip2)[1];
if (@OP@(in1r, in1i, in2r, in2i) || npy_isnan(in2r) || npy_isnan(in2i)) {
if (npy_isnan(in2r) || npy_isnan(in2i) || @OP@(in1r, in1i, in2r, in2i)) {
((@ftype@ *)op1)[0] = in1r;
((@ftype@ *)op1)[1] = in1i;
}
Expand All @@ -2795,6 +2785,7 @@ NPY_NO_EXPORT void
((@ftype@ *)op1)[1] = in2i;
}
}
npy_clear_floatstatus_barrier((char*)dimensions);
}
/**end repeat1**/

Expand Down
10 changes: 4 additions & 6 deletions numpy/core/src/umath/simd.inc.src
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1014,7 +1014,7 @@ sse2_@kind@_@TYPE@(@type@ * ip, @type@ * op, const npy_intp n)
{
const npy_intp stride = 16 / (npy_intp)sizeof(@type@);
LOOP_BLOCK_ALIGN_VAR(ip, @type@, 16) {
*op = (*op @OP@ ip[i] || npy_isnan(*op)) ? *op : ip[i];
*op = (npy_isnan(*op) || *op @OP@ ip[i]) ? *op : ip[i];
}
assert(n < (stride) || npy_is_aligned(&ip[i], 16));
if (i + 3 * stride <= n) {
Expand All @@ -1038,15 +1038,13 @@ sse2_@kind@_@TYPE@(@type@ * ip, @type@ * op, const npy_intp n)
}
else {
@type@ tmp = sse2_horizontal_@VOP@_@vtype@(c1);
*op = (*op @OP@ tmp || npy_isnan(*op)) ? *op : tmp;
*op = (npy_isnan(*op) || *op @OP@ tmp) ? *op : tmp;
}
}
LOOP_BLOCKED_END {
*op = (*op @OP@ ip[i] || npy_isnan(*op)) ? *op : ip[i];
}
if (npy_isnan(*op)) {
npy_set_floatstatus_invalid();
*op = (npy_isnan(*op) || *op @OP@ ip[i]) ? *op : ip[i];
}
npy_clear_floatstatus_barrier((char*)op);
}
/**end repeat1**/

Expand Down
18 changes: 8 additions & 10 deletions numpy/core/tests/test_half.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,21 +301,19 @@ def test_half_ufuncs(self):
assert_equal(np.copysign(b, a), [2, 5, 1, 4, 3])

assert_equal(np.maximum(a, b), [0, 5, 2, 4, 3])
with suppress_warnings() as sup:
sup.record(RuntimeWarning)
x = np.maximum(b, c)
assert_(np.isnan(x[3]))
assert_equal(len(sup.log), 1)

x = np.maximum(b, c)
assert_(np.isnan(x[3]))
x[3] = 0
assert_equal(x, [0, 5, 1, 0, 6])

assert_equal(np.minimum(a, b), [-2, 1, 1, 4, 2])
with suppress_warnings() as sup:
sup.record(RuntimeWarning)
x = np.minimum(b, c)
assert_(np.isnan(x[3]))
assert_equal(len(sup.log), 1)

x = np.minimum(b, c)
assert_(np.isnan(x[3]))
x[3] = 0
assert_equal(x, [-2, -1, -np.inf, 0, 3])

assert_equal(np.fmax(a, b), [0, 5, 2, 4, 3])
assert_equal(np.fmax(b, c), [0, 5, 1, 4, 6])
assert_equal(np.fmin(a, b), [-2, 1, 1, 4, 2])
Expand Down
5 changes: 1 addition & 4 deletions numpy/core/tests/test_regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1557,10 +1557,7 @@ def test_ticket_1434(self):

def test_complex_nan_maximum(self):
cnan = complex(0, np.nan)
with suppress_warnings() as sup:
sup.record(RuntimeWarning)
assert_equal(np.maximum(1, cnan), cnan)
assert_equal(len(sup.log), 1)
assert_equal(np.maximum(1, cnan), cnan)

def test_subclass_int_tuple_assignment(self):
# ticket #1563
Expand Down
19 changes: 8 additions & 11 deletions numpy/core/tests/test_umath.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1327,21 +1327,18 @@ def test_lower_align(self):
assert_equal(d.max(), d[0])
assert_equal(d.min(), d[0])

def test_reduce_warns(self):
def test_reduce_reorder(self):
# gh 10370, 11029 Some compilers reorder the call to npy_getfloatstatus
# and put it before the call to an intrisic function that causes
# invalid status to be set. Also make sure warnings are emitted
# invalid status to be set. Also make sure warnings are not emitted
for n in (2, 4, 8, 16, 32):
for dt in (np.float32, np.float16, np.complex64):
with suppress_warnings() as sup:
sup.record(RuntimeWarning)
for r in np.diagflat(np.array([np.nan] * n, dtype=dt)):
assert_equal(np.min(r), np.nan)
assert_equal(len(sup.log), n)

def test_minimize_warns(self):
# gh 11589
assert_warns(RuntimeWarning, np.minimum, np.nan, 1)
for r in np.diagflat(np.array([np.nan] * n, dtype=dt)):
assert_equal(np.min(r), np.nan)

def test_minimize_no_warns(self):
a = np.minimum(np.nan, 1)
assert_equal(a, np.nan)


class TestAbsoluteNegative(object):
Expand Down
0