BUG: Potential fix for divmod(1.0, 0.0) to raise divbyzero and return inf #16161
Conversation
|
Let's limit this PR to the float case, as that has a fairly trivial "correct" behavior (match |
|
@eric-wieser I agree, we can limit this to float for now. I sent out an email on the list for a discussion about the same. |
|
I somewhat expect we need to check for |
isn't this expected ? as i raised in the mailing list, I was not sure about whether divmod should set both flags. I thought that since it does both div and mod operations it should set both flags . the ieee754 standard doesnt make a specific comment about this particular operation insists that division or remainder individually should set only one flag. |
5ea2a86 to
54905ca
Compare
|
The test failures seem to be because of #16220 and 1.0%0.0 operations not raising invalid warning on mac. I was looking for platform specific tests that omit macOS, but couldnt find any. Is this the first time we are encountering this ? |
|
just so that people don't get misled by this (since I put the comment in the wrong place).
the above comment is for the current ci failures on mac with clang. It fails because it doesn't raise the invalid warning on clang builds, since clang doesn't set the invalid flag for 1.0 % 0.0 operations. |
482b043 to
4f52add
Compare
|
Any idea on where I should be looking to fix this CI error : "ValueError: path is on mount 'D:', start on mount 'C:'". It happened twice in this PR, most likely not related to this PR. |
|
No, its failing everywhere today... I thought we fixed something along these lines on master recently, but apparently not, or not robustly. |
4f52add to
b7afc41
Compare
|
np.fmod(1.0, 0.0) I think will likely fail on Macos. I have added tests for fmod to check. not that its in the divmod code path but maybe good to fix. Current status:
EDIT: np.fmod now works on mac os. |
|
This looks about ready, is it good to go from your perspective @anirudh2290? |
|
Thanks for the ping @rgommers ! From my perspective this was ready. Although, the pytorch PR related to raising a domain error for integer np.fmod(1, 0) operations is something this PR doesn't address. It addresses the changes given in this table:
It also addresses some behavior changes between different compiler versions for nan or inf usages in these operations. Earlier, this was compiler dependent, now we force set the invalid and divide by zero flags for the operations in the table and it should behave uniformly when compiled with gcc-5 or gcc-8 / gcc-9 for example. |
|
This could use a release note under |
|
Yeah, this should really be good. There were some smaller reorganization tries I was curious about, but never went down the road, I will try to put that on my TODO list to finish this off. |
|
@anirudh2290 Could you add a release note? Look in |
|
I like to make the table part of the release notes. Pandoc translates it to rst without problems but I'd like to check the result first. Anyone know a good linux viewer for rst? |
|
@charris: try pasting the text below into https://livesphinx.herokuapp.com/ |
|
Putting this in, I'll make another PR for the release note. |
DOC: Add release note for gh-16161.
|
Apologies for missing the notification here. Thank you for merging this @charris :) |
This potentially fixes: #14900 . This is currently WIP until discussion concludes on #14900. According to IEEE754 standard this 1 / 0 double division should return inf. Note: currently in numpy, np.divmod(1.0, 0.0) and np.divmod(1, 0) don't return consistent results, its (inf, nan) for former and (0, 0) for latter.
I have currently limited the change to floating point divmod. This will require more changes for int_
divmod to return (inf, nan) (probably has back. compatibility implications) ,I am guessing here : https://github.com/numpy/numpy/blob/master/numpy/core/src/umath/loops.c.src#L909EDIT: inf cannot be returned for int_ divmod , we have to choose between current behavior and raising an error.