Skip to content
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

Jump to bottom

ENH: issue overflow warning when using abs on np.int8(-128) #21648

Merged
merged 18 commits into from
Jun 13, 2022
Merged

ENH: issue overflow warning when using abs on np.int8(-128) #21648

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
511240f
ENH Issue overflow warning when using `abs` on `np.int8(-128)`
Micky774 Jun 2, 2022
1ef5d3d
Update numpy/core/src/umath/scalarmath.c.src
Micky774 Jun 2, 2022
756b852
Added safer check for overflow
Micky774 Jun 3, 2022
84bbdc8
Simplified check
Micky774 Jun 3, 2022
74e9aff
Merge branch 'main' into absolute_byte_overflow
Micky774 Jun 4, 2022
41aa046
Fixes/reenables test
Micky774 Jun 4, 2022
08e2d33
Added fperror check to `@type@_absolute`
Micky774 Jun 4, 2022
d8925f5
Linting
Micky774 Jun 4, 2022
ca8f199
Merge branch 'main' into absolute_byte_overflow
Micky774 Jun 10, 2022
5155f57
Removed extra FPError handling
Micky774 Jun 10, 2022
76e53ac
Merge branch 'main' into absolute_byte_overflow
Micky774 Jun 10, 2022
4c379d1
Fixed integer division FP_OVERFLOW
Micky774 Jun 10, 2022
3eac1ac
Typo
Micky774 Jun 10, 2022
19d84d7
Caught warnings for negating unsigned integers
Micky774 Jun 10, 2022
c513aa7
Merge branch 'main' into absolute_byte_overflow
Micky774 Jun 12, 2022
42adc88
Enabled tests
Micky774 Jun 12, 2022
754062a
Marked `xfail` for multiplication due to error on Windows
Micky774 Jun 12, 2022
3e692d8
MAINT: Revert change to division (and mark test as skip)
seberg Jun 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 22 additions & 2 deletions numpy/core/src/umath/scalarmath.c.src
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ static NPY_INLINE int
}
#if @neg@
else if (b == -1 && a == NPY_MIN_@NAME@) {
*out = a / b;
*out = NPY_MIN_@NAME@;
Copy link
Member

Choose a reason for hiding this comment

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

I think #21507 will cover it so we can remove this here perhaps? All though I'm happy to rebase if it goes in first.

return NPY_FPE_OVERFLOW;
Copy link
Member

Choose a reason for hiding this comment

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

This is suspicious: is this line really not hit during tests?

Copy link
Member

Choose a reason for hiding this comment

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

Yep #21507 has tests that covers this.

Copy link
Member

Choose a reason for hiding this comment

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

I think both PRs have tests for this, so suspect that it is just in-lining/optimization leading to coverage errors (that might be the first time I have seen coverage failing, but...).

@ganesh-k13 if you prefer it to not be included here, maybe just push the change yourself? (We have to mark that one additional test as skip then probably.) Otherwise, I am happy to merge as-is.

Yes, it would be nice to follow-up to expand the tests so that they raise the error, and there is still lots of little stuff to clear out in multiplication... But at this point it feels like iterating here is getting tricky.

}
#endif
Expand Down Expand Up @@ -584,10 +584,15 @@ static NPY_INLINE int
/**begin repeat
* #name = byte, short, int, long, longlong#
* #type = npy_byte, npy_short, npy_int, npy_long, npy_longlong#
* #NAME = BYTE, SHORT, INT, LONG, LONGLONG#
*/
static NPY_INLINE int
@name@_ctype_absolute(@type@ a, @type@ *out)
{
if (a == NPY_MIN_@NAME@) {
*out = a;
return NPY_FPE_OVERFLOW;
}
Micky774 marked this conversation as resolved.
Show resolved Hide resolved
*out = (a < 0 ? -a : a);
return 0;
}
Expand Down Expand Up @@ -1564,8 +1569,23 @@ static PyObject *


val = PyArrayScalar_VAL(a, @Name@);
int retstatus = @name@_ctype_@oper@(val, &out);

@name@_ctype_@oper@(val, &out);
if (retstatus) {
int bufsize, errmask;
PyObject *errobj;

if (PyUFunc_GetPyValues("@name@_scalars", &bufsize, &errmask,
&errobj) < 0) {
return NULL;
}
int first = 1;
if (PyUFunc_handlefperr(errmask, errobj, retstatus, &first)) {
Py_XDECREF(errobj);
return NULL;
}
Py_XDECREF(errobj);
}
Copy link
Member

Choose a reason for hiding this comment

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

It is suspicious that these new lines are not marked as being tested

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, right. The first one is OK (I don't think it can be reasonably covered, it indicates an internal/critical error).

To hit the second one, we should also check for:

with np.errstate(over="raise"):
    with pytest.raises(FloatingPointError):
         operation


/*
* TODO: Complex absolute should check floating point flags.
Expand Down
2 changes: 1 addition & 1 deletion numpy/core/tests/test_scalarmath.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,7 @@ def test_scalar_signed_integer_overflow(dtype, operation):

@pytest.mark.parametrize("dtype", np.typecodes["UnsignedInteger"])
@pytest.mark.xfail # TODO: the check is quite simply missing!
Micky774 marked this conversation as resolved.
Show resolved Hide resolved
def test_scalar_signed_integer_overflow(dtype):
def test_scalar_unsigned_integer_overflow(dtype):
val = np.dtype(dtype).type(8)
with pytest.warns(RuntimeWarning, match="overflow encountered"):
-val
Expand Down
6 changes: 4 additions & 2 deletions numpy/typing/tests/data/pass/arithmetic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from typing import Any
import numpy as np
import pytest

c16 = np.complex128(1)
f8 = np.float64(1)
Expand Down Expand Up @@ -330,8 +331,9 @@ def __rpow__(self, value: Any) -> Object:
-f4
-i8
-i4
-u8
-u4
with pytest.warns(RuntimeWarning):
-u8
-u4
-td
-AR_f

Expand Down