-
-
Notifications
You must be signed in to change notification settings - Fork 26k
MNT co-vendor array-api-{compat, extra} #30340
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
b53a032
to
0ebef66
Compare
This comment was marked as outdated.
This comment was marked as outdated.
thanks for the script example @thomasjpfan, that seems to have worked! @betatim could I get a responsible adult to add the GPU CI label please 😁 |
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.
oops, converting to draft
@lucascolley I have a questions about integrating with array-api-extra. If we are implementing the array API for a particular function or class and in the process there seems to be some function that is not implemented in the array API specification and we need to override it, if we implement it in array-api-extra instead of how we are doing it currently in the utils, won't this be a blocker until the next version of array-api-extra is released? |
It doesn't have to be instead of utils. I would suggest you keep your current workflow the exact same - you can still put private helpers in utils to start off with. But in addition, every now and then, you can offload them to array-api-extra so that others can use them. That's what we're doing in SciPy. |
Okay I understand. Thank you. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as 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.
A few comments but generally 👍 from me.
I think this makes the install story nicer and solves a problem I had been pondering but didn't know a good solution to: which versions of array-api-compat should we support? Thanks for mentioning that Lucas!
Thanks all, I count +4! Is anyone else wanting to check this over before it acquires a merge conflict? |
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. Thank you @lucascolley
Just a few points:
- Is diff1d the only function that we can utilize from array-api-extra right now as we have a bunch of them in the array api utils?
- We might need to take an additional responsibility to either open an issue on array-api-extra or add the functionality ourselves whenever we add a new function in the scikit-learn array api utils?
It is worth checking http://data-apis.org/array-api-extra/api-reference.html. We should be able to transfer a lot over from sklearn,
That would be appreciated! |
@lucascolley Here is a list of functions I observed that could be moved. However I think if we want to integrate them it should be done in a follow up PR after this is merged. Let me know what you think about these:
|
Yep, feel free to open issues (or PRs) over at array-api-extra and we can discuss! |
Maybe @thomasjpfan has an opinion as the original author of many of the array API helper tooling of scikit-learn. |
@lucascolley can you re-run the lock file generation script? The weekly update happened today so we now have conflicts :-/ Should we wait a few more days and then merge? Technically we have enough 👍 already, but maybe worth waiting a moment longer? But not too long so we don't make Lucas sort out conflicts repeatedly |
Co-authored-by: Guido Imperiale <crusaderky@gmail.com>
🫡 |
Required by scikit-learn#30340.
Required by scikit-learn#30340.
Required by scikit-learn#30340.
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.
We did not initially vendor array-api-compat
because they were releasing bug fixes quickly and scikit-learn has a longer release cycle.
I am +1 on vendoring both array-api-{compat, extra}
.
thanks all! |
Co-authored-by: Guido Imperiale <crusaderky@gmail.com>
closes gh-30367
SCIPY_ARRAY_API
being setarray_api_compat.numpy
instead of_NumPyAPIWrapper
xpx.setdiff1d
TODO: