Skip to content

ENH Array Api support for linear, polynomial and sigmoid kernels#29475

Merged
adrinjalali merged 4 commits into
scikit-learn:mainfrom
OmarManzoor:array_api_linear_poly_sigmoid_kernels
Aug 28, 2024
Merged

ENH Array Api support for linear, polynomial and sigmoid kernels#29475
adrinjalali merged 4 commits into
scikit-learn:mainfrom
OmarManzoor:array_api_linear_poly_sigmoid_kernels

Conversation

@OmarManzoor
Copy link
Copy Markdown
Contributor

@OmarManzoor OmarManzoor commented Jul 12, 2024

Reference Issues/PRs

Towards #26024

What does this implement/fix? Explain your changes.

  • Adds array api support for linear, polynomial and sigmoid kernels in sklearn.pairwise

Any other comments?

CC: @ogrisel @adrinjalali @betatim

Note: I did check that the CUDA tests seem to pass.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 12, 2024

✔️ Linting Passed

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

Generated for commit: 5122ff8. Link to the linter CI: here

@OmarManzoor OmarManzoor added Array API Quick Review For PRs that are quick to review labels Jul 12, 2024
Copy link
Copy Markdown
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 checked the code for polynomial_kernel and linear_kernel and they indeed seem to be ok if left unchanged, so just adding them to the array API test suite should indeed be enough.

Assuming CI is still green after conflict resolution, LGTM.

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Aug 19, 2024

The CUDA run was green:

https://github.com/scikit-learn/scikit-learn/actions/runs/10451046790/job/28936627899

but the status was removed from the list once I added the "waiting for reviewer" label.

@ogrisel ogrisel added the Waiting for Second Reviewer First reviewer is done, need a second one! label Aug 19, 2024
@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Aug 19, 2024

There was a random 403 error calling fetch_california_housing on pylatest_conda_forge_mkl. Re-running.

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Aug 19, 2024

@betatim this one should be quick to review and merge and would unblock the other linked PR.

@OmarManzoor
Copy link
Copy Markdown
Contributor Author

@adrinjalali Could you kindly review this PR? I think it should be quick review.

if gamma is None:
gamma = 1.0 / X.shape[1]

K = safe_sparse_dot(X, Y.T, dense_output=True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm confused, safe_sparse_dot seems to support array API, but at the end we have

https://github.com/OmarManzoor/scikit-learn/blob/5122ff8bbbe0f1d4a3ba7da62d73de69968f46c5/sklearn/utils/extmath.py#L212

But I can't find toarray in the array API standard. So what am I missing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That part of the code only runs for sparse arrays. So it isn't linked to the array api.

@adrinjalali adrinjalali merged commit 23fc332 into scikit-learn:main Aug 28, 2024
@OmarManzoor OmarManzoor deleted the array_api_linear_poly_sigmoid_kernels branch August 28, 2024 11:46
MarcBresson pushed a commit to MarcBresson/scikit-learn that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Array API module:metrics Quick Review For PRs that are quick to review Waiting for Reviewer Waiting for Second Reviewer First reviewer is done, need a second one!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants