-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
ENH: Add floating point error checking to (almost) all casts #21437
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
Conversation
Hmm, looking at the failures:
EDIT: OK, So about 3, valgrind actually does point to an incorrectly free'd tuple (or a double free even), which would add up... Unfortunately, it only shows at interpreter shutdonw. |
0f48e16
to
a6c07a7
Compare
63dd8c5
to
c640bba
Compare
This should be ready for review now. I think it maybe should have a release not (so tagging). There are still one or two error paths that I will try to cover in tests, but overall I think I got it. There are some tricky parts here. The first five commits may be good to go through on their own. But overall I will be happy to look at the changes together. The by far worst part I think is the advanced indexing... EDIT: Hmmm, surprising the test fails only on windows 32bits, but seems that this may not be a fluke but a real refcounting (or cleanup timing/race condition?) issue somewhere. (Shouldn't matter too much for a first review though.) |
The code itself is a lot of changes to pass the resultant error state around and looks fine. I think it would be worthwhile to put this in now. Do you know if it breaks any scipy/astropy tests? |
Thanks for the review and slashing down the release note. I would like a note somewhere that float to int casts may return undefined values, but its not really right in this note (it is unchanged anyway). I have not tested downstream test-suites (I may look into that tomorrow or so). OTOH, I don't expect larger problems, and they would be fixable with |
To achieve this, I first need to propagate the ArrayMethod information that FPE flags _may_ be given for a specific cast. This requires quite a bit of changes (which is arguably the more annoying part of the PR/commit). Note that for some "legacy" casts, we do not modify this and assume that the only interesting information is `needs_api`.
unfortunately, I had to realize that float -> integer casts are not well defined when values are out of range for the integer. This is a problem with C, but means that neither the warnings seem to be particularly well defined... (I suspect, on my CPU it either warns OR gives "valid" integer overflow results, but the question is if that is even universally true...)
The test meant to assert shapes, but didn't actually do it.
Just ran the SciPy 1.8.1 test-suite on this. It has 16 (related) failures, out of which 15 are all in the same EDIT: There are likely a lot more warnings elsewhere which are just not floated for a released version of SciPy! |
I am a little hesitant to merge a PR that we know for sure will break SciPy tests. Could you open an issue that refers to this PR? |
Not sure if a NumPy or SciPy issue. Happy to followup with either quickly though. I tried checking astropy. Not that the run went perfectly (too old or new pytest version), but it does not look like astropy is affected. |
Thanks @seberg |
TEST: Suppress new numpy warning on nan-to-int cast numpy/numpy#21437 does not require us to change our behavior, but does require us to change our expectations of what warnings are raised. Since this is quite an edge case, it seems to make more sense to suppress the warning and eventually expect it when numpy 1.24 becomes our minimum.
This, unfortunately larger, PR tries to add floating point error checking to all places where this is necessary. Unfortunately, there are a few places, the most complicated one being the advanced indexing (simply because there are many branches – note that only assignments are truly relevant).
Doing this also required threading it into the
NpyIter
, the way this is used from elsewhere is a cludge (due to being non-public API).There are a few smaller problems (most of which we can hopefully ignore):
ufunc.at
, but that has its own problems anyway, so I am not too concerned about it.There is still some cleanup and maybe simplification necessary, so marking as draft. But this will close a few open issues, and it ensures the warning for
np.float32(2e200)
.So, I think it is both a pretty worthwhile thing, and is one of those warnings that I would like to have available when we/if we switch to "weak Python scalar" promotion (remove value based casting).