-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
DISCUSS: Runtime precision selection drive? #23362
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
Comments
This seems like a good idea. We already have a start for disabling SIMD features in order to preserve byte-for-byte compatible results, this seems like a natural extension. |
I think the context manager part is partially done, at least errstate can already be used as one. |
It is a context manager, but not backed by a context variable, which has its own set of bugs, so it felt like if we extend that mechanism that might make sense. I am not sure if it is necessary though, I admit. |
There was further discussion on the mailing list. I think there is consensus that the current 1.25.0rc2 errors for sin and cos are too large, and that we should revert. @r-devulap what should we set the permitted ULP error to here numpy/numpy/core/tests/data/generate_umath_validation_data.cpp Lines 113 to 115 in e75214c
2.37, 3.3 / 2.36, 3.38 ? Then we should regenerate the data files. As I understand the code we should change these lines to avoid using the SIMD code for sin/cos
|
Rather than revert, I'd rather just disable. Long term, I'm hoping for either/both of:
|
The "fast" context manger would have one more advantage: Disabling FPEs for it is more of a no-brainer. |
Sorry, I did not properly communicate my proposal. We should revert the code to the situation we had before SVML by disabling the SIMD code paths for |
Sounds like we are in agreement then. There don't seem to be any other unfixed problems with 1.25.0rc1, so I will release after the fix goes in. |
I will work on submitting a PR: disable SIMD for sin/cos, modify ULP error for sin/cos to 1 and regenerate data files. |
@r-devulap thanks, much appreciated. 👏 |
Are we reversing both float32 and float64 SIMD versions? float32 version does not use SVML and has an upper ULP bound of 1.49 (has been around since #13368). |
Just the new float64 version. There have been no complaints about the float32 version and 1.49 ULPS seems reasonable. |
makes sense. |
See #23925 |
Since
|
Yesterday there was a bit of a discussion around adding an env variable to allow runtime selection (mainly opt out of lower precision SVML). I need to think it a bit over, but overall, I would be fine with doing that.
There was also a discussion that a proper solution may be to allow selecting this maybe as a context (or similar). I don't think that is a small project, but I am not sure it is huge either.
I am not planning on attacking it, but if e.g. @mattip is interested in it together with @seiko2plus it may be a well scoped project, so I thought I open an issue to see if things drop out.
What would it take? Some internal reorganization for sure:
We probably need to fixnp.errstate
to be a context variable (that is needed anyway). I think we could then expand it to tag on a "float precision" context?np.errstate
is now a contextvar with ENH,API: Make the errstate/extobj a contextvar #23936. This makes it now easier to extend and absolutely no worry to fetch the internalnpy_extobj
once and pass it around to anywhere needed. (It should be very fast to fetch now, so even multiple fetches are fine in practice.)context
passed into ufuncs andget_loop
.The first thing is well scoped, but a mid-sized project (not too hard really). The second thing should be very easy afterwards, but may need refactoring of the
errstate
handling as a follow-up to the first.The second point is easy enough.
For the third point, we end up here currently: https://github.com/numpy/numpy/blob/main/numpy/core/src/umath/legacy_array_method.c#L195
that function can be customized freely for each ufunc. And the float context is even passed through to the inner-loop (but you need to customize that function to use the new-style inner-loop signature).
Now, we need to customize the
get_loop()
, which is a bit annoying in the current machinery. But I am not sure it is that bad: Matti effectively did the same for theufunc.at
loop specialization already.Note that I am happy to leave
ufunc->functions[i]
function pointer that we currently use/fill atNULL
. Nobody should be using that! (This part might need very managable followups if you want to keep supportingpnumpy
style things, but I suspect that is a bridge that can be crossed when someone asks for it.)(I am happy to close this, if there is no explicit interest.)
The text was updated successfully, but these errors were encountered: