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

Micky774 · 2022-06-02T20:44:58Z

Related Issues/PRs

Resolves #21289

Changes

Checks condition a == NPY_MIN_@NAME@ to determine whether an overflow error has occurred for np.int8 type. See #21289 and #21188 (comment) for reference.

Comments

This is my first PR and it's not-quite-complete yet but I wanted to open it so that I could gather some feedback and assistance. In particular, I'm not sure if this change is sufficient, nor how to write tests for this that adhere to NumPy standards. Any help would be appreciated!

numpy/core/src/umath/scalarmath.c.src

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

numpy/core/src/umath/scalarmath.c.src

Micky774 · 2022-06-04T03:27:48Z

Couple things I found that I have addressed in this PR (trying to avoid too large a scope):

There was no fperror handling for absolute in scalarmath.c so I added it to the .src file.
signed vs unsigned overflow tests were set up incorrectly. In particular, test_scalar_unsigned_integer_overflow was actually titled test_scalar_signed_integer_overflow and overwrote it (not sure if intentional?)
The test_scalar_signed_integer_overflow test fails on integer floor division, so I set that as xfail to deal with in a follow-up PR.

ganesh-k13 · 2022-06-05T06:24:54Z

Just mentioning #21507 as it handles a few cases.

seberg

Thanks, seems like it should be pretty much ready, the main thing is that I would prefer omitting the unnecessary floating point flag handling.

numpy/core/src/umath/scalarmath.c.src

seberg · 2022-06-10T21:21:11Z

numpy/core/tests/test_scalarmath.py

@@ -898,7 +898,8 @@ def test_scalar_integer_operation_overflow(dtype, operation):
 @pytest.mark.parametrize("operation", [
        lambda min, neg_1: abs(min),
        lambda min, neg_1: min * neg_1,
-        lambda min, neg_1: min // neg_1], ids=["abs", "*", "//"])
+        pytest.param(lambda min, neg_1: min // neg_1,
+            marks=pytest.mark.xfail)], ids=["abs", "*", "//"])


Probably need to comment it out or skip this, because it is just too broken...

At the risk of slightly expanding the scope of this PR, I went ahead and just avoided the actual error-causing computation in the integer division function, setting it to return NPY_MIN_@NAME@ as I think matches gcc behavior (in lieu of true defined behavior, this is the next-best thing imo). The test runs now, and the behavior matches expectations I think.

OK, I would like to backport these division fixes at some point. But I am starting to suspect we should make a PR with those from scratch… (i.e. a backport only PR)

seberg · 2022-06-10T23:06:34Z

@ganesh-k13 maybe you want to review/finalize this? @Micky774, unfortunately, I don't quite believe you have double-checked that the tests check everything as advertised :). Can you please make sure we have tests for this new abs path (and the negative one for that matter). Tests that fail without the changes applied.

EDIT: To be clear, modulo making sure the tests actually exist/do what they should, I think this can go in.

numpy/core/tests/test_scalarmath.py

mattip · 2022-06-12T13:17:46Z

numpy/core/src/umath/scalarmath.c.src

@@ -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@;
        return NPY_FPE_OVERFLOW;


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

Yep #21507 has tests that covers this.

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.

mattip · 2022-06-12T13:19:12Z

numpy/core/src/umath/scalarmath.c.src

+            return NULL;
+        }
+        Py_XDECREF(errobj);
+    }


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

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

ganesh-k13

Sorry I missed the ping Sebastian, yeah the changes look ok but seems like Azure pipelines shows tests failed for windows builds that need to be checked.

ganesh-k13 · 2022-06-12T13:27:24Z

numpy/core/src/umath/scalarmath.c.src

@@ -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@;


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.

seberg · 2022-06-12T17:41:07Z

Gah, this is a mess... The linux code (mostly) uses npy_mul_with_overflow_@name@ which is probably great and correct when the builtin is available. When it is not available, it seems just plain +wrong for signed inputs.
The overflow checks themselves seem all pretty bogus to be honest? Just xfail the test? We should fix it, but not here... Maybe same with the division test (to be able to pull out the change as Ganesh said).

Sorry that this is a a bit of never-ending story, because these integer overflow checks are so spotty, right now.

Micky774 · 2022-06-12T18:01:05Z

Gah, this is a mess... The linux code (mostly) uses npy_mul_with_overflow_@name@ which is probably great and correct when the builtin is available. When it is not available, it seems just plain +wrong for signed inputs. The overflow checks themselves seem all pretty bogus to be honest? Just xfail the test? We should fix it, but not here... Maybe same with the division test (to be able to pull out the change as Ganesh said).

Sorry that this is a a bit of never-ending story, because these integer overflow checks are so spotty, right now.

I just spent the last couple hours eventually finding that check and scratching my head at it, so I appreciate the confirmation that I'm not going insane XD

Yeah it seems there's some systemic revisions necessary here. I suppose the least-invasive, most-likely-to-not-idle result would be to:

Maintain the current changes for abs
Maintain the enabling of the signed-overflow test (via the rename)
Maintain the enabling of the unsigned-overflow test (via removal of xfail)
xfail the multiplication test on the signed-overflow test to defer to a later PR
Tentatively be okay w/ some lack of code coverage, depending on BUG: Better report integer division overflow #21507

Does that sound about right?

And thanks to all of you for your patience with this 😄

This is because similar changes are already included in a different PR that fixes up more places where division may have problems.

seberg · 2022-06-13T19:10:38Z

OK, just added that commit myself, so that this is again focused only on abs and adding the FPE raising logic (as it also affects negative partially).

I am going to merge it once the tests pass, thanks @Micky774 and Ganesh. @Micky774 it would be cool to look into the open issues here:

-min_signed_int was missing the check I think?
It would be cool if those tests (well my failed attempt to write some ;)), gets fixed up to also run the code with np.errstate(all="raise") to cover the error path.
I really would like to fix up the multiplication, that requires a bit of thought and its own PR though! (not as important as fixing the division stuff, but also somewhat important NEP 50!).

seberg · 2022-06-13T20:53:38Z

Only the ppc64le job isn't finished yet not going to wait for that. Thanks.

Related Issues/PRs Follow-up to #21648 Changes Adds check to scalar negative for determining whether an overflow warning should be raised for signed integers. Improves testing for negative operator on unsigned integers.

seberg · 2022-06-18T13:12:20Z

xref gh-21506 (for an eventual release note summarizing these).

…ication (#21793) Remane `npy_mul_with_overflow_intp` to `npy_mul_sizes_with_overflow` as it only allows positive numbers. Then introduce new versions for all integers (not intp) to use it for the integer scalar math with overflow detection. (It is OK to use everywhere, just for sizes we know they will be positive normally.) Related to #21506 Follow-up to #21648

ENH Issue overflow warning when using abs on np.int8(-128)

511240f

github-actions bot added the 01 - Enhancement label Jun 2, 2022

seberg added the 59 - Needs tests label Jun 2, 2022

seberg reviewed Jun 2, 2022

View reviewed changes

numpy/core/src/umath/scalarmath.c.src Outdated Show resolved Hide resolved

Update numpy/core/src/umath/scalarmath.c.src

1ef5d3d

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

WarrenWeckesser reviewed Jun 2, 2022

View reviewed changes

numpy/core/src/umath/scalarmath.c.src Outdated Show resolved Hide resolved

Added safer check for overflow

756b852

Micky774 mentioned this pull request Jun 3, 2022

MAINT: improved overflow check to avoid undefined behavior #21658

Merged

WarrenWeckesser reviewed Jun 3, 2022

View reviewed changes

numpy/core/src/umath/scalarmath.c.src Outdated Show resolved Hide resolved

Micky774 added 5 commits June 3, 2022 10:57

Simplified check

84bbdc8

Merge branch 'main' into absolute_byte_overflow

74e9aff

Fixes/reenables test

41aa046

Added fperror check to @type@_absolute

08e2d33

Linting

d8925f5

seberg reviewed Jun 6, 2022

View reviewed changes

numpy/core/src/umath/scalarmath.c.src Outdated Show resolved Hide resolved

numpy/core/src/umath/scalarmath.c.src Show resolved Hide resolved

seberg added 59 - Needs tests and removed 59 - Needs tests labels Jun 8, 2022

Micky774 added 3 commits June 10, 2022 12:56

Merge branch 'main' into absolute_byte_overflow

ca8f199

Removed extra FPError handling

5155f57

Merge branch 'main' into absolute_byte_overflow

76e53ac

seberg reviewed Jun 10, 2022

View reviewed changes

Micky774 added 3 commits June 10, 2022 17:35

Fixed integer division FP_OVERFLOW

4c379d1

Typo

3eac1ac

Caught warnings for negating unsigned integers

19d84d7

seberg reviewed Jun 10, 2022

View reviewed changes

numpy/core/tests/test_scalarmath.py Outdated Show resolved Hide resolved

seberg approved these changes Jun 10, 2022

View reviewed changes

mattip reviewed Jun 12, 2022

View reviewed changes

ganesh-k13 reviewed Jun 12, 2022

View reviewed changes

ganesh-k13 mentioned this pull request Jun 12, 2022

BUG: Better report integer division overflow #21507

Merged

Micky774 added 2 commits June 12, 2022 10:43

Merge branch 'main' into absolute_byte_overflow

c513aa7

Enabled tests

42adc88

Micky774 and others added 2 commits June 12, 2022 14:04

Marked xfail for multiplication due to error on Windows

754062a

MAINT: Revert change to division (and mark test as skip)

3e692d8

This is because similar changes are already included in a different PR that fixes up more places where division may have problems.

seberg merged commit cadfd83 into numpy:main Jun 13, 2022

Micky774 deleted the absolute_byte_overflow branch June 14, 2022 02:42

Micky774 mentioned this pull request Jun 17, 2022

ENH: add overflow handling for scalar negative operation #21790

Merged

Micky774 mentioned this pull request Jun 18, 2022

ENH: Add overflow handling for negative integers scalar multiplication #21793

Merged

ngoldbaum mentioned this pull request Sep 18, 2023

BUG: Overflow reported when negating uint64 #24737

Closed

mhvk mentioned this pull request Mar 27, 2024

BUG/question: Should np.abs always return a positive number? #26145

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Micky774 commented Jun 2, 2022 •

edited

Micky774 commented Jun 4, 2022

ganesh-k13 commented Jun 5, 2022

seberg left a comment

seberg Jun 10, 2022

Micky774 Jun 10, 2022

seberg Jun 10, 2022

seberg commented Jun 10, 2022 •

edited

mattip Jun 12, 2022

ganesh-k13 Jun 12, 2022

seberg Jun 13, 2022

mattip Jun 12, 2022

seberg Jun 12, 2022

ganesh-k13 left a comment

ganesh-k13 Jun 12, 2022

seberg commented Jun 12, 2022 •

edited

Micky774 commented Jun 12, 2022

seberg commented Jun 13, 2022

seberg commented Jun 13, 2022

seberg commented Jun 18, 2022

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

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

Conversation

Micky774 commented Jun 2, 2022 • edited

Related Issues/PRs

Changes

Comments

Micky774 commented Jun 4, 2022

ganesh-k13 commented Jun 5, 2022

seberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented Jun 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganesh-k13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented Jun 12, 2022 • edited

Micky774 commented Jun 12, 2022

seberg commented Jun 13, 2022

seberg commented Jun 13, 2022

seberg commented Jun 18, 2022

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

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

Micky774 commented Jun 2, 2022 •

edited

seberg commented Jun 10, 2022 •

edited

seberg commented Jun 12, 2022 •

edited