Skip to content

ENH add support for Array API to mean_pinball_loss and explained_variance_score #29978

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.

Sign up for GitHub

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

Jump to bottom
Merged
OmarManzoor merged 15 commits into from
Dec 3, 2024
Merged

ENH add support for Array API to mean_pinball_loss and explained_variance_score #29978

OmarManzoor merged 15 commits into from
Dec 3, 2024

Conversation

@virchan
Copy link
Member

@virchan virchan commented Sep 30, 2024 •
edited
Loading

Reference Issues/PRs

Towards #26024.

What does this implement/fix? Explain your changes.

This PR adds Array API support for the following regression metrics:

  • median_absolute_error (dependent on _weighted_percentile)
  • explained_variance_score
  • d2_pinball_score (dependent on percentile and _weighted_percentile)
  • d2_absolute_error_score (dependent on percentile and _weighted_percentile)
  • mean_pinball_loss

Any other comments?

cc: @ogrisel

@github-actions
Copy link

github-actions bot commented Sep 30, 2024 •
edited
Loading

✔️ Linting Passed

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

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

@virchan virchan marked this pull request as ready for review October 1, 2024 22:55
@glemaitre glemaitre self-requested a review October 17, 2024 18:08
@glemaitre glemaitre mentioned this pull request Oct 17, 2024
Merged
glemaitre
glemaitre reviewed Oct 17, 2024
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

So I think that the functions relying on the percentile or _weighted_percentile function cannot be implemented for the moment.

@virchan virchan force-pushed the issues/26024/regression_metrics branch from 394a527 to cf41a3e Compare October 18, 2024 02:30
@virchan virchan closed this Oct 18, 2024
@virchan virchan force-pushed the issues/26024/regression_metrics branch from cf41a3e to d2acd79 Compare October 18, 2024 02:42
@virchan virchan reopened this Oct 18, 2024
@virchan virchan changed the title ENH add support for Array API to various regression metrics ENH add support for Array API to mean_pinball_loss and explained_variance_score Oct 18, 2024
@virchan
Copy link
Member Author

virchan commented Oct 18, 2024

So I think that the functions relying on the percentile or _weighted_percentile function cannot be implemented for the moment.

TL;DR: I've reverted the metrics. Thank you for catching that!

I have resolved the dependence of percentile and _weighted_percentile for the metrics. Only mean_pinball_loss and explained_variance_score will be updated.

I'll also add a note in the Array API meta issue to inform others to skip median_absolute_error, d2_pinball_score, and d2_absolute_error_score until the functions are implemented.

@virchan virchan mentioned this pull request Oct 18, 2024
Open
glemaitre
glemaitre reviewed Oct 18, 2024
View reviewed changes
ogrisel
ogrisel reviewed Oct 18, 2024
View reviewed changes
This was referenced Oct 19, 2024
Closed
Merged
glemaitre
glemaitre approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@glemaitre glemaitre 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 @virchan

@OmarManzoor
Copy link
Contributor

OmarManzoor commented Dec 2, 2024

@virchan Could you kindly resolve the conflicts and also sync now that #30128 has been merged?

@virchan
Copy link
Member Author

virchan commented Dec 2, 2024

Thanks for the updates @OmarManzoor! It's ready for a second review!

OmarManzoor
OmarManzoor previously approved these changes Dec 3, 2024
View reviewed changes
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.

Otherwise LGTM

@github-actions github-actions bot removed the CUDA CI label Dec 3, 2024
@OmarManzoor OmarManzoor self-requested a review December 3, 2024 06:20
OmarManzoor
OmarManzoor reviewed Dec 3, 2024
View reviewed changes
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.

@virchan Two of the CUDA tests are failing. I think we need to handle using the appropriate device. Could you kindly have a look?

@OmarManzoor OmarManzoor dismissed their stale review December 3, 2024 06:22

CUDA tests are failing

@virchan
Copy link
Member Author

virchan commented Dec 3, 2024

Thank you for the follow-up, @OmarManzoor!

I have updated the explained_variance_score function, and the CUDA test looks fine locally. Could you please run it again on your end?

@github-actions github-actions bot removed the CUDA CI label Dec 3, 2024
@OmarManzoor
Copy link
Contributor

OmarManzoor commented Dec 3, 2024

@virchan Were you able to test with the MPS device locally?

OmarManzoor
OmarManzoor approved these changes Dec 3, 2024
View reviewed changes
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 @virchan

I checked with MPS as well and it looks okay 🟢

@OmarManzoor OmarManzoor merged commit 8ded7f4 into scikit-learn:main Dec 3, 2024
34 checks passed
@virchan
Copy link
Member Author

virchan commented Dec 3, 2024

Thank you everyone for your time!

virchan added a commit to virchan/scikit-learn that referenced this pull request Dec 9, 2024
@virchan
ENH add support for Array API to mean_pinball_loss and `explained_v…
a10ca24 
…ariance_score` (scikit-learn#29978)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ogrisel ogrisel ogrisel left review comments

@glemaitre glemaitre glemaitre approved these changes

@OmarManzoor OmarManzoor OmarManzoor approved these changes

Assignees

No one assigned

Labels

Array API module:metrics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@virchan @OmarManzoor @ogrisel @glemaitre