Skip to content

ENH Add array_api compatibility to max_error#29212

Merged
betatim merged 3 commits intoscikit-learn:mainfrom
EdAbati:max_error_array_api
Jun 13, 2024
Merged

ENH Add array_api compatibility to max_error#29212
betatim merged 3 commits intoscikit-learn:mainfrom
EdAbati:max_error_array_api

Conversation

@EdAbati
Copy link
Contributor

@EdAbati EdAbati commented Jun 7, 2024

Reference Issues/PRs

Towards #26024

What does this implement/fix? Explain your changes.

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

@github-actions
Copy link

github-actions bot commented Jun 7, 2024

✔️ Linting Passed

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

Generated for commit: 0e88c7d. Link to the linter CI: here

@EdAbati
Copy link
Contributor Author

EdAbati commented Jun 7, 2024

Had to change the type of y_true in the regression test from int to float because of this

Was there a reason why that was int?

@EdAbati
Copy link
Contributor Author

EdAbati commented Jun 7, 2024

mps and CUDA tests are green on my side :)

@EdAbati EdAbati marked this pull request as ready for review June 8, 2024 09:27
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.

@ogrisel ogrisel added the Quick Review For PRs that are quick to review label Jun 12, 2024
@ogrisel
Copy link
Member

ogrisel commented Jun 12, 2024

Thanks again for you Array API PRs @EdAbati.

Since you already had quite a bit of experience on those I was wondering if you wouldn't be interested in contributing on PRs that are a bit less well defined but potentially more impactful from a performance point of view?

For instance I think we could improve the array API support for the solver="cholesky" case of the Ridge class by leveraging the xp.linalg.solve standard API.

@EdAbati
Copy link
Contributor Author

EdAbati commented Jun 13, 2024

Hi @ogrisel , sounds good!

I was also looking at dcg_score that, after a brief look, seems to require things are not implemented yet.
I will also make a draft PR with that, so we can discuss

@betatim
Copy link
Member

betatim commented Jun 13, 2024

I triggered the CUDA CI for this PR https://github.com/scikit-learn/scikit-learn/actions/runs/9500016241

(Mostly for my own sake to use the CI a bit)

@betatim betatim merged commit bd8f5bd into scikit-learn:main Jun 13, 2024
@EdAbati EdAbati deleted the max_error_array_api branch June 15, 2024 07:22
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
11 tasks
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants