-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
MAINT: Disable SIMD version of float64 sin and cos #23925
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
I don't think the CI failures are related to the patch, right? |
I reran the problematic CI checks and they passed. |
@pllim and I are confused: we thought that with the disabling done in this PR, EDIT: See astropy/astropy#14946 |
I don't have the architecture to check AVX512, but 1.25.0 works fine with what I have. The problematic code should have been disabled in the 1.25.0 release, it is in the changelog. Maybe we haven't got it quite right yet? |
I don't have AVX512 either, and cannot reproduce the failures we see... Will try to find something more up to date. |
I don't think it is >>> import numpy as np
>>> np.__version__
'1.25.0'
>>> np.cos(0) # OK
1.0
>>> # [0, 45, 90] deg
>>> res = np.cos([0, 0.78539816, 1.57079633])
>>> np.testing.assert_allclose(res, [1, 0.707107, 0]) # Traceback Particularly... For 45 deg:
For 90 deg:
|
|
Not enough digits in the input.
|
The point is our tests that used to pass with numpy 1.24 or older is still not passing after this reversion in numpy 1.25. Sure, I can try exact pi divided by something, but I still get disagreement in numpy 1.25. Is no one else seeing this? >>> res = np.cos([0, np.pi / 4, np.pi / 2])
>>> res
array([1.00000000e+00, 7.07106781e-01, 6.12323400e-17])
>>> np.testing.assert_allclose(res[1], 0.707107) # traceback, see below
>>> np.testing.assert_allclose(res[2], 0) # traceback, see below
|
But I'll wait for @mhvk to comment since he knows the original design of tests that are failing better than I do. |
@pllim I am not sure what is going on. But the values you are posted are the same I have on NumPy 1.23 on linux: I.e. those values seem like the correctly rounded results. Exactness can only be expected for |
Yeah I noticed a similar thing (see below) but I got distracted from posting until now:
Just a little surprising we couldn't do a clean revert, but it also could be caused by our own changes. 🤷 |
This is the actual test case and if you look at the blame, we have not touched it for 6 years but yet only now it fails: testcase(
f=np.cos,
q_in=(np.array([0.0, np.pi / 4.0, np.pi / 2.0]) * u.radian,),
q_out=(np.array([1.0, 1.0 / np.sqrt(2.0), 0.0]) * u.one,),
), |
Ah, could be I missed something in the reversion... hang on. |
Okay, I think disagreement with numpy 1.25 is addressed (my bad!), however... did you un-revert this in 2.0.dev? Or are the failures here (only appear when we pull in numpy "nightly" wheel) something else? Thanks! https://github.com/astropy/astropy/actions/runs/5324760647/jobs/9644536347?pr=14946 |
Rather than revert, I left the code in place.