-
Notifications
You must be signed in to change notification settings - Fork 16
ENH: setdiff1d: add delegation #456
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
ENH: setdiff1d: add delegation #456
Conversation
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.
see also #124
|
Two issues have been identified during the development of the JAX part (see jax-ml/jax#32335 and jax-ml/jax#32402 ) I will finish my contribution once all the issues have been reported to the JAX library. |
|
According to my understanding of the work discussed in #124, the JAX JIT version has been postponed indefinitely, but the Dask version is still desired. Since the function setdiff1d is not available in the Dask API (see Dask NumPy compatibility), the implementation proposed by @crusaderky seems worth pushing forward. Because this implementation differs from a classic delegate function (as it introduces a new approach), where should I place the dedicated code for Dask? Alternatively, do we want to replace the current version, which uses the |
|
I'm not sure about how to proceed for Dask and JAX, but I would be happy to review a PR which just adds delegation for the other libraries. |
6887ffb to
62f3d6c
Compare
320a1d2 to
2f1eadb
Compare
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.
LGTM, thanks @Enderdead !
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.
Just a nit on the tests.
I checked that the documentation builds correctly.
|
@crusaderky Corrections made to the tests. |
|
Did you mean to include a change to |
|
@lucascolley I swapped the function order in the file to follow alphabetical order and added the missing item to the |
|
all good, thanks again @Enderdead ! And for the review @crusaderky |
This merge request introduces the
setdiff1ddelegate function (#100).Nothing special to mention here — it was NOT a straightforward contribution.