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

MAINT, SIMD: Handle overflow gracefully in integer division #21727

Merged
merged 3 commits into from Jun 12, 2022
Merged

MAINT, SIMD: Handle overflow gracefully in integer division #21727

merged 3 commits into from Jun 12, 2022

Conversation

ganesh-k13
Copy link
Member

Handle SIMD division overflow

Merge before: #21507
Part of: #21506

Raising this separately to not blow up the original PR on scalars.

Changes:

Before:

>>> import numpy as np
>>> np.array([np.iinfo(np.int32).min]*10, dtype=np.int32) // np.int32(-1)
<stdin>:1: RuntimeWarning: divide by zero encountered in floor_divide
array([0, 0, 0, 0, 0, 0, 0, 0, 0, 0], dtype=int32)

Now:

>>> import numpy as np
>>> np.array([np.iinfo(np.int32).min]*10, dtype=np.int32) // np.int32(-1)
<stdin>:1: RuntimeWarning: overflow encountered in floor_divide
array([-2147483648, -2147483648, -2147483648, -2147483648, -2147483648,
       -2147483648, -2147483648, -2147483648, -2147483648, -2147483648],
      dtype=int32)

@ganesh-k13 ganesh-k13 requested a review from seberg June 10, 2022 18:49
@ganesh-k13 ganesh-k13 added this to the 1.23.1 release milestone Jun 10, 2022
@ganesh-k13
Copy link
Member Author

Hey @seberg, thought of going bottom up on this one. This solves the SIMD overflows, once merged, I'll fix the scalars and arrays for other ops along with the tests.

seberg
seberg reviewed Jun 10, 2022
View reviewed changes
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Thanks, happy with doing this one-by-one. I am slightly wondering if/how we should backport it, but maybe that is better seen later.

LGTM modulo the test fixup. Will wait a bit in case someone more familiar with SIMD wants to have a look.

numpy/core/tests/test_umath.py Outdated
else:
a // b
if fo.min and fo.min in a:
with pytest.raises(FloatingPointError):
with pytest.raises(RuntimeWarning):
Copy link
Member

Choose a reason for hiding this comment

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

Rather than changing this, change the np.errstate to include overflow='raise'. (This is actually necessary, the warning will not be raised in release mode.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh ok, thanks!

numpy/core/src/umath/loops_arithmetic.dispatch.c.src Outdated
if (d == 0) {
npy_set_floatstatus_divbyzero();
return 0;
} else if ((n == NPY_MIN_@TYPE@) && (d == -1)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} else if ((n == NPY_MIN_@TYPE@) && (d == -1)) {
}
else {

Maybe its clear and we don't have to repeat it? (otherwise moving the else would be nice).

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe chain it like above? In any case, feel free to ignore, the newline comment if this is used more in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah you are right, shouldn't code after 12 AM :)

@charris
Copy link
Member

charris commented Jun 10, 2022

At this point I don't want to do anymore backports except straight bug fixes.

@seberg
Copy link
Member

seberg commented Jun 10, 2022 •
edited

Yeah, agreed. It may make sense to do a very minimal bug-fix that just solves the crashes. But maybe lets figure out this stuff here first and then do that?

@seberg seberg removed this from the 1.23.1 release milestone Jun 10, 2022
@ganesh-k13
BUG, SIMD: Handle division overflow errors
0307d43 
* If a types minimum value is divided by -1, an overflow will be raised
  and result will be set to minimum
@ganesh-k13
TST: Modified tests to reflect new overflow
fccb91f
ganesh-k13
ganesh-k13 commented Jun 11, 2022
View reviewed changes
numpy/core/src/umath/loops_arithmetic.dispatch.c.src
npyv_@sfx@ cvtozero = npyv_select_@sfx@(error, vzero, vneg_one);
warn = npyv_or_@sfx@(error, warn);
warn_zero = npyv_or_@sfx@(bzero, warn_zero);
warn_overflow = npyv_or_@sfx@(overflow, warn_overflow);
Copy link
Member Author

Choose a reason for hiding this comment

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

I hope VSX is covered as part of the CI. Else would be nice to get @seiko2plus opinion on this?

seberg
seberg approved these changes Jun 11, 2022
View reviewed changes
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM now. Will give it a bit time in case of SIMD related review.

@charris
Copy link
Member

charris commented Jun 11, 2022

The test failure can be ignored.

@seberg @rafaelcfsousa
Update numpy/core/src/umath/loops_arithmetic.dispatch.c.src
71b0dfa 
Co-authored-by: Rafael Sousa <90851201+rafaelcfsousa@users.noreply.github.com>
@seberg seberg changed the title SIMD: Handle division overflow SIMD: Handle overflow gracefully in integer division Jun 12, 2022
@seberg seberg merged commit 7052444 into numpy:main Jun 12, 2022
@rgommers rgommers added the component: SIMD Issues in SIMD (fast instruction sets) code or machinery label Jul 12, 2022
@charris charris changed the title SIMD: Handle overflow gracefully in integer division MAINT, SIMD: Handle overflow gracefully in integer division Sep 7, 2022
@charris charris added 03 - Maintenance 09 - Backport-Candidate PRs tagged should be backported labels Sep 7, 2022
@ganesh-k13 ganesh-k13 mentioned this pull request Sep 8, 2022
Merged
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seberg seberg seberg approved these changes

@rafaelcfsousa rafaelcfsousa rafaelcfsousa left review comments

Assignees
No one assigned
Labels
03 - Maintenance component: SIMD Issues in SIMD (fast instruction sets) code or machinery
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ganesh-k13 @charris @seberg @rafaelcfsousa @rgommers