-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
TST: array types: wrap namespaces centrally #22320
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 had assumed the purpose of using un-wrapped namespaces in the tests was for the tests to better reflect the conditions in practice, assuming end-users do not use That said, since From that perspective, providing a wrapped |
Correct.
Unless a user-facing function accepts xp as an optional parameter and the test explicitly passes |
I am not aware of any. |
I think this works, yes. I suppose if array-api-compat ever did modify the array objects then we would have problems.
https://docs.scipy.org/doc/scipy/reference/generated/scipy.fft.fftfreq.html |
You also need a test that explicitly passes the unwrapped xp, otherwise it will default internally to |
assert xp_size(xp.unique_values(res.nit)) == 4 | ||
assert xp_size(xp.unique_values(res.nfev)) == 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails in CI PyTorch, stating that res.nit has 3 unique elements. Can't reproduce locally.
https://github.com/scipy/scipy/actions/runs/12767945483/job/35587336176?pr=22320
To me it seems like a genuine bug but I don't have enough domain knowledge to understand what's right and wrong. @mdhaber could you have a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a bug. I'll fix the test in a bit . You can skip that line if it's holding you up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skipped, ty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking more carefully, this part of the test was NumPy-only before the PR:
scipy/scipy/differentiate/tests/test_differentiate.py
Lines 589 to 591 in df134ea
if is_numpy(xp): | |
assert len(np.unique(res.nit)) == 4 | |
assert len(np.unique(res.nfev)) == 4 |
The purpose is to check that it's possible for all four numbers to be different. It was challenging to find an example for which that was the case, and it turned out to be sensitive to arithmetic, so we made it NumPy only.
It must have started failing only because the if is_numpy
line was removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It must have started failing only because the if is_numpy line was removed?
Correct. I find surprising that different backends can yield different results.
Ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @crusaderky, very nice clean-up
Co-authored-by: Lucas Colley <lucas.colley8@gmail.com>
Prevent headaches such as https://github.com/scipy/scipy/pull/22240/files#r1913475509