BUG: Fix warning problems of the mod operator #19316
Conversation
|
Could you fix up the tests (and maybe even add new ones to describe what you changed?). That also clarifies exactly what changed for the reviewers. |
|
@seberg did it! Check the changes when you can please. |
| @@ -716,9 +710,6 @@ npy_divmod@c@(@type@ a, @type@ b, @type@ *modulus) | |||
| @type@ div, mod, floordiv; | |||
|
|
|||
| /* force set invalid flag, doesnt raise by default on gcc < 8 */ | |||
There was a problem hiding this comment.
This line should probably go, too.
There was a problem hiding this comment.
This is my "problem" so to speak. I am confused what this is about and what we tried to do in gh-16161.
My guess is that this godbolt snippet explains the comment if you switch to GCC 8, the correct answer is given (warning flag is set).
Now... If a is NaN and b < 0, the if (mod) will evaluate True if mod is NaN. Especially, if both a and b are NaN, then we definitely take that branch and both b < 0 and mod < 0 evaluate to False with setting the warning (since they are both NaN). And that was wrong on GCC 7, which did not set the warning (but led to the better result).
(Hopefully, that flow analysis is right... But something is off and my best guess is to fix most of these comparisons and replace them with isgreater and isless, as mentioned before and now also in gh-19398. In general, I don't think we really ever want float comparisons to set warning flags really, so de-facto all of those in the math code are probably just wrong...)
I honestly don't understand why:
with np.errstate(invalid='raise'):
assert_no_warnings(FloatingPointError, np.floor_divide, 1, np.nan)
succeeds! If I just call np.floor_divide(1, np.nan), I get an "invalid value" warning, and with np.errstate(invalid="raise") the FloatingPointError.
There was a problem hiding this comment.
@arubiales I am confused, did you maybe try these things out on a gcc version older than 8 (Which would be pretty old)?
There was a problem hiding this comment.
@nschloe Yes sorry I fogot to delete it.
@seberg No my GCC version is 8.4. I tryed the same, and with the changes I have no warning, but I'm with you, should raise the "invalid value" flag. After test with the following change:
NPY_INPLACE @type@
npy_floor_divide@c@(@type@ a, @type@ b) {
@type@ div, mod;
if (NPY_UNLIKELY(!b)) {
div = a / b;
if (!a || npy_isnan(a)) {
npy_set_floatstatus_invalid();
} else {
npy_set_floatstatus_divbyzero();
}
} else {
div = npy_divmod@c@(a, b, &mod);
/* This is the change, I just set the invalid float status in the else */
npy_set_floatstatus_invalid();
}
I have the "invalid" flag Which mean I never enter in the if because NPY_UNLIKELY(!b) is always False
There was a problem hiding this comment.
Yeah, !b is false, but then it goes into divmod? I have to check more, maybe I was also missing a git clean...
Can you do me a favor and delete the assert_no_warnings completely and make it np.errstate(all="raise")? I still have to think about it more. I guess this is a good change though, and we should have more fixes as followup.
|
Perhaps and |
|
@nschloe Do you want to add it in this PR? A new test function as for example |
|
I have pushed a test commit to remove I am thinking about how to fix things, but I am not sure how easy it will be to confine changes to a minimal set here. |
|
Sorry, for taking over a bit here, but I had to look at it with much more concentration to make progress. It is a bit tricky to split the @charris maybe you can have a look? I think this should be fine now, although a couple of new tests might be good (Although Anirudh had added good tests and the only problem is the behaviour with NaNs coming in). The things to remember is:
The last point is a big reason for the whole confusion here:
|
| } | ||
| #endif | ||
| /**end repeat1**/ | ||
|
|
There was a problem hiding this comment.
These fmod implementations were added in the old divmod-warnings PR. Removing them undoes that change.
9d75d18 to
a0060d7
Compare
| * but set the error flags correct (divmod may set too many). | ||
| * (Using !b here, since reverse logic set incorrect flags on clang) | ||
| */ | ||
| return npy_fmod@c@(a, b); |
There was a problem hiding this comment.
What I had missed before is that this also fails the following test for divmod:
numpy/numpy/core/tests/test_umath.py
Lines 579 to 580 in 6496f1d
fmod(1, 0) seems to simply not set the correct floating point flags on clang (at least in the version used by CI). I suppose we could work around clang (or even mark the test as xfail?)
06ae9ee to
5277577
Compare
Don't worry @seberg , you have more knowledge that me about Numpy. I'm following all your changes, if there is something I can help, please tell me, I will be happy to help! |
ebf46a2 to
428fae5
Compare
|
Thanks. Right now I am thinking of calling it done. The tests will fail, and I will fix them up so that it skips the shaky ones at least on MacOS. (With a good chance of more skips needed for weird platforms.) I have two options:
I dislike testing for correct flags a bit. It feels like this is bound to fail on exotic platforms. I am honestly wondering if rather than failing, these tests should just print out: 8000 "Careful, your system does not follow IEEE warning flags well for divmod" |
The old tests seems not to have noticed all types of errors here. It seems the `assert_no_warnings` context does not care about exceptions.
It is very much possible that this will also fail on other systems and should rather only run, e.g. on newer glibc versions. In principle, we could also split this into "no warnings given" and "warning given". MacOS seems very sloppy with floating point warnings, but hopefully it at least does not give spurious ones.
428fae5 to
e502de2
Compare
|
We discussed this at the triage meeting and decided to merge this as is. In a follow on PR we should mention that macOS (probably)/clang (less likely) is not IEEE compliant. |
|
Thanks @seberg |
That's the initial solution, there are 13 test failing because it does not trigger the warning that we are resolving. So If this solution is correct I think we should change the test too.