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.

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

BUG: Better report integer division overflow (backport) #22230

Merged

Conversation

ganesh-k13
Copy link
Member

@ganesh-k13 ganesh-k13 commented Sep 8, 2022

Backport of #21507 and #21727.

Handle SIMD division overflow

Merge before: #21507
Part of: #21506

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

Changes: #21727

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)

Changes: #21507

  • Handle overflow cases
  • Testcases for same

Related: #21506
Finishes: #19260

cc: @seberg @rafaelcfsousa

ganesh-k13 and others added 8 commits September 8, 2022 18:36
Overflows for remainder/divmod/fmod

* If a types minimum value is divided by -1, an overflow will be raised
  and result will be set to minimum
* Handle overflow and return 0 in case of mod related operations
Co-authored-by: Rafael Cardoso Fernandes Sousa <rafaelcfsousa@ibm.com>
* Changed `raise` to `warns` and test for `RuntimeWarning`
* Added results check back
This introduces a helper to iterate through "interesting" array cases
that could maybe be used in other places.  Keep the other test intact,
it adds a check for mixed types (which is just casts currently, but
cannot hurt) and is otherwise thorough.
I am not certain the unlikely cases make much sense to begin with,
but they are certainly not helpful within an unlikely block.
@ganesh-k13 ganesh-k13 added this to the 1.23.3 release milestone Sep 8, 2022
@seberg
Copy link
Member

seberg commented Sep 8, 2022

Ah, this part applies well! The scalar paths probably need manual work as the code is actually different unfortunately... (those were different PRs, although that probably hardly matters due to the changes in main)

@@ -156,16 +156,25 @@ static NPY_INLINE int
* #type = npy_byte, npy_ubyte, npy_short, npy_ushort, npy_int, npy_uint,
* npy_long, npy_ulong, npy_longlong, npy_ulonglong#
* #neg = (1,0)*5#
* #NAME = BYTE, UBYTE, SHORT, USHORT, INT, UINT,
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 yes, it was a small change in one of the loop definitions, didn't expect backport to fail as I only checked the diff :), hopefully, works now.

@ganesh-k13
Copy link
Member Author

Oddly the tests passed locally, but the error seems valid as we might need to backport another PR if I remember correctly. Taking a look.

ganesh-k13 and others added 3 commits September 8, 2022 19:12
* If a types minimum value is divided by -1, an overflow will be raised
  and result will be set to minimum
Co-authored-by: Rafael Sousa <90851201+rafaelcfsousa@users.noreply.github.com>
@ganesh-k13
Copy link
Member Author

Ok this needed backport of #21727 as well, I have added the commits here itself.

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, not so bad after all :). This does change super niche warnings, but I think we can risk that one considering how very niche it is.

The new tests should be very extensive, so I do not think we have to worry about having forgotten something. It would be great if you could double check the SIMD results, though @rafaelcfsousa.

@rafaelcfsousa
Copy link
Contributor

The tests are passing without errors on POWER/VSX4. Thank you!

@charris charris added the 08 - Backport Used to tag backport PRs label Sep 8, 2022
@charris charris merged commit 9bf22bb into numpy:maintenance/1.23.x Sep 8, 2022
@charris
Copy link
Member

charris commented Sep 8, 2022

Thanks @ganesh-k13 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug 08 - Backport Used to tag backport PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants