Skip to content

ENH Add Array API compatibility for additive_chi2_kernel #29144

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jun 13, 2024
Merged

ENH Add Array API compatibility for additive_chi2_kernel #29144

merged 8 commits into from
Jun 13, 2024

Conversation

Tialo
Copy link
Contributor

@Tialo Tialo commented May 30, 2024 •
edited
Loading

Towards #26024

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Due to _chi2_kernel_fast written in cython it can't be used with array-api containers. So I implemented all calculations using array broadcasting. It is not faster than cython function, so I use _chi2_kernel_fast when additive_chi2_kernel gets a numpy array.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 30, 2024 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: edef1c9. Link to the linter CI: here

ogrisel
ogrisel approved these changes Jun 7, 2024
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Could you please just update the _array_api.rst documentation file to reference this function?

@ogrisel
Copy link
Member

ogrisel commented Jun 7, 2024

And there are a bunch of conflicts to resolve when merging main.

@ogrisel
Copy link
Member

ogrisel commented Jun 7, 2024

Also please check that the CUDA tests pass with https://gist.github.com/EdAbati/ff3bdc06bafeb92452b3740686cc8d7c.

@Tialo
Copy link
Contributor Author

Tialo commented Jun 7, 2024

CUDA tests passed

@ogrisel ogrisel added Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one! labels Jun 12, 2024
@ogrisel
Copy link
Member

ogrisel commented Jun 12, 2024

Ping @betatim @OmarManzoor.

@betatim
Copy link
Member

betatim commented Jun 13, 2024 •
edited
Loading

I resolved the merge conflict and started the CUDA CI https://github.com/scikit-learn/scikit-learn/actions/runs/9501959732

edit: CI failed because I made a mistake when resolving the conflict.

@betatim betatim merged commit a1027b5 into scikit-learn:main Jun 13, 2024
30 checks passed
@Tialo Tialo deleted the array-api/additive_chi2_kernel branch June 13, 2024 16:05
@OmarManzoor OmarManzoor mentioned this pull request Jun 14, 2024
Closed
@OmarManzoor
Copy link
Contributor

OmarManzoor commented Jun 14, 2024 •
edited
Loading

@ogrisel @betatim A test seemed to be failing i.e. the one using Torch mps. I have proposed a fix here #29256. Could you kindly confirm that the error occurs and the fix is valid?

@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
Merged
11 tasks
@OmarManzoor OmarManzoor mentioned this pull request Sep 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@betatim betatim betatim approved these changes

Assignees
No one assigned
Labels
Array API module:metrics Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
Array API
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Tialo @ogrisel @betatim @OmarManzoor