Skip to content

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).

@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
@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
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
11 tasks
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 2, 2024
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.

3 participants