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 Add Array API compatibility to cosine_similarity #29014

Merged
merged 10 commits into from
May 17, 2024

Conversation

EdAbati
Copy link
Contributor

@EdAbati EdAbati commented May 14, 2024

Reference Issues/PRs

Towards #26024

What does this implement/fix? Explain your changes.

It makes thecosine_similarity implementation compatible and tested with the Array API.

Please let me know how I can improve it :)

cc @ogrisel @betatim

Copy link

github-actions bot commented May 14, 2024

✔️ Linting Passed

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

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

@EdAbati EdAbati marked this pull request as ready for review May 14, 2024 19:40
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.

Thanks for the PR @EdAbati. However it already needs an update with conflict resolution w.r.t. the main branch.

Do you have a chance to run the tests on a CUDA host or do you want somebody else to do it? (I can do it).

sklearn/metrics/pairwise.py Show resolved Hide resolved
@EdAbati
Copy link
Contributor Author

EdAbati commented May 15, 2024

Thank you @ogrisel for the quick reply!

Do you have a chance to run the tests on a CUDA host or do you want somebody else to do it? (I can do it).

Yes, I tested on CUDA in Google Colab and on my Mac (using mps). All green on my side ✅

Screenshot 2024-05-15 at 18 47 54

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 once the proposed argument renaming discussed above is implemented.

@EdAbati
Copy link
Contributor Author

EdAbati commented May 17, 2024

Thank you very much!:)

Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @EdAbati

@OmarManzoor OmarManzoor merged commit b461547 into scikit-learn:main May 17, 2024
29 checks passed
@OmarManzoor
Copy link
Contributor

@EdAbati Just for some guidance, how did you run this code in google colab so that we can test with gpus?

@EdAbati
Copy link
Contributor Author

EdAbati commented May 17, 2024

Thanks again @OmarManzoor :)

I just published the Google Colab notebook in a gist: https://gist.github.com/EdAbati/ff3bdc06bafeb92452b3740686cc8d7c

@EdAbati EdAbati deleted the cosine-array-api branch May 17, 2024 16:20
@OmarManzoor
Copy link
Contributor

Thanks again @OmarManzoor :)

I just published the Google Colab notebook in a gist: https://gist.github.com/EdAbati/ff3bdc06bafeb92452b3740686cc8d7c

Thank you for sharing!

@jeremiedbb jeremiedbb mentioned this pull request May 20, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants