[go: up one dir, main page]

Skip to content
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 Array API support for euclidean_distances and rbf_kernel #29433

Merged

Conversation

OmarManzoor
Copy link
Contributor

Reference Issues/PRs

Towards #26024

What does this implement/fix? Explain your changes.

  • Adds array api support for euclidean_distances and rbf_kernel

Any other comments?

Copy link
github-actions bot commented Jul 8, 2024

✔️ Linting Passed

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

Generated for commit: 526d932. Link to the linter CI: here

@OmarManzoor
Copy link
Contributor Author
OmarManzoor commented Jul 9, 2024

@ogrisel Don't we have the latest version of array api strict on all CI pipelines? This seems to be failing for some pipelines probably because of the older version of array-api-strict, while it works fine on my local system with the latest version of array-api-strict. Should we fix this just for supporting an old version?

@ogrisel
Copy link
Member
ogrisel commented Jul 9, 2024

@OmarManzoor let me update this PR with main after the merge of #29388.

Further CI fixes will follow-up once #29436 is reviewed and merged.

@OmarManzoor
Copy link
Contributor Author
OmarManzoor commented Jul 9, 2024

@ogrisel These CI errors seem a bit weird. One is related to the docstring of get_namespace_and_device. The other error is related to a casting issue which does not occur on my local system. Edit: I see that the second error is in a docstring. I am checking it.

sklearn/metrics/pairwise.py Outdated Show resolved Hide resolved
sklearn/metrics/pairwise.py Outdated Show resolved Hide resolved
sklearn/utils/_array_api.py Outdated Show resolved Hide resolved
sklearn/utils/extmath.py Outdated Show resolved Hide resolved
@ogrisel
Copy link
Member
ogrisel commented Jul 9, 2024

I thought I fixed that docstring failure somewhere else already.

EDIT: here it is:

https://github.com/scikit-learn/scikit-learn/pull/29318/files#diff-6264adab84df6fafdb2d950141783b52b718da1e1be773daae7b5921b2556f94R572

@OmarManzoor feel free to steal this from the Ridge PR since I won't have time to finalize it this week.

@ogrisel
Copy link
Member
ogrisel commented Jul 9, 2024

Please feel free to merge main into this branch to benefit from the fixes in #29436.

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.

I labeled "CUDA CI" to trigger a CI run with a CUDA GPU.

If everything is green, LGTM.

doc/modules/array_api.rst Outdated Show resolved Hide resolved
doc/modules/array_api.rst Outdated Show resolved Hide resolved
@ogrisel ogrisel added CUDA CI and removed CUDA CI labels Jul 10, 2024
@ogrisel
Copy link
Member
ogrisel commented Jul 10, 2024

I tried to relabel this PR to trigger the CUDA CI out of curiosity but it's still waiting on workers for some unknown reason:

Copy link
Member
@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM. Thanks @ogrisel for the ping.

sklearn/metrics/pairwise.py Outdated Show resolved Hide resolved
sklearn/metrics/pairwise.py Outdated Show resolved Hide resolved
Copy link
Member
@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

LGTM. I'll let @ogrisel have another look and merge.

@OmarManzoor
Copy link
Contributor Author

@ogrisel Can this be merged?

sklearn/utils/_array_api.py Outdated Show resolved Hide resolved
@ogrisel ogrisel enabled auto-merge (squash) July 11, 2024 14:51
@ogrisel ogrisel merged commit e7af195 into scikit-learn:main Jul 11, 2024
28 checks passed
@OmarManzoor OmarManzoor deleted the array_api_euclidean_distances_rbf_kernel branch July 11, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants