-
Notifications
You must be signed in to change notification settings - Fork 5
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
TST/BUG: run all tests on all backends; fix backend-specific bugs #88
Conversation
@@ -54,7 +54,9 @@ def in1d( | |||
order = xp.argsort(ar, stable=True) | |||
reverse_order = xp.argsort(order, stable=True) | |||
sar = xp.take(ar, order, axis=0) | |||
if sar.size >= 1: | |||
ar_size = _compat.size(sar) | |||
assert ar_size is not None, "xp.unique*() on lazy backends raises" |
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.
Caveat: this is missing data-apis/array-api-compat#231
|
||
# mypy: disable-error-code=no-untyped-usage |
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.
The return type of xp.anything is untyped, because xp is a ModuleType
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.
I looked over everything apart from the changes under tests/
to start off with - seems like a good idea, thanks!
src/array_api_extra/testing.py
Outdated
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.
could you detail how these functions deviate from the ones in scipy._lib._array_api
? If we are going to expose these here, it would be good to be able to remove them from SciPy.
Also, consider #17 (comment). But given we have since changed the scope of array-api-extra to include paths specific to known backends, I think they probably are in-scope now.
EDIT: given
Note that I have no plans to use these in scipy, as there are enough extra nuances
there to warrant bespoke variants.
perhaps there is a way to make these extensible for library-specific nuances, to remove code-duplication somewhat?
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.
- no default_ctx context variable
- no ability to cherry-pick disabling shape, dtype, and/or namespace validation
- no special casing for 0d
All of these features could be catered for here, but I feel they're all severely overdesigned for ensuring the health of array-api-extra.
My goal was to write something that serves well array-api-extra, and as a nice bonus allows other people to use it.
I'm open to making the whole new module private if you'd prefer it that way.
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.
let's keep it private for now and explain the situation in gh-17. If there are requests for them to be made public in the future, we should reconsider.
Also, see #72 for my vision of how I would like to include delegation to library-specific implementations more broadly. The testing introduced here should be enough to push that PR over the line too 🎉 |
4c2ec53
to
7445917
Compare
Made the functions private. |
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 is fantastic, thanks @crusaderky !
create_diagonal
on JAXkron
on JAX, CuPy, and PyTorchpad
on JAXin1d
on PyTorchpublicprivate functionsxp_assert_equal
andxp_assert_close
.Note that I have no plans to use these in scipy, as there are enough extra nuances
there to warrant bespoke variants.