Skip to content

ENH Add Array API compatibility to zero_one_loss and accuracy_score#27137

Merged
ogrisel merged 14 commits intoscikit-learn:mainfrom
EdAbati:zero-one-loss-array-api
Sep 7, 2023
Merged

ENH Add Array API compatibility to zero_one_loss and accuracy_score#27137
ogrisel merged 14 commits intoscikit-learn:mainfrom
EdAbati:zero-one-loss-array-api

Conversation

@EdAbati
Copy link
Copy Markdown
Contributor

@EdAbati EdAbati commented Aug 22, 2023

Reference Issues/PRs

Towards #26024

What does this implement/fix? Explain your changes.

It makes the zero_one_loss and accuracy_score(since it was a dependency) implementations compatible and tested with the Array API.

Any other comments?

  • I have added a test for _weighted_sum. When having sample_weights as floats I got the following from PyTorch: 
    >           return float(sample_score @ sample_weight)
E           RuntimeError: dot : expected both vectors to have same dtype, but found Double and Float
  • It feels like I am missing some cases in the tests. For example, should I test with a y_true and y_pred that is not from binary classification? 🤔

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 22, 2023 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 51258ed. Link to the linter CI: here

ogrisel
ogrisel reviewed Aug 23, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ogrisel ogrisel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR. See the reply to your question in the comments below:

OmarManzoor
OmarManzoor reviewed Aug 23, 2023
View reviewed changes
@OmarManzoor
Copy link
Copy Markdown
Contributor

OmarManzoor commented Aug 23, 2023

@ogrisel I think I'll wait for this PR to be merged before working on other metrics, so that the addition of tests doesn't conflict or become redundant. What do you think?

@EdAbati
Copy link
Copy Markdown
Contributor Author

EdAbati commented Aug 30, 2023 •
edited
Loading

Thank you everyone for the feedback. :) I have updated the PR

@ogrisel , should we test the multilabel case? Or when y is multilabel, is it always transformed into a sparse matrix?
(is_multilabel does not seem fully compatible with the ArrayAPI yet, because of this)

  • Is there a better place for _array_api_atol since it is just used for testing?
  • likewise, _array_api_for_tests is in utils.estimators_checks, but it is used for metrics checks too. Should we move it to _array_api or somewhere else?

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Aug 30, 2023

If think we can leave the multilabel out of the scope of this PR. Not sure hiw we could tackle it.

I for the atol helper, we can put in the same module as other array api helpers for now. Since it's private we can always change later.

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Aug 30, 2023

Ok for moving _array_api_for_tests, as well.

@betatim betatim mentioned this pull request Aug 30, 2023
Closed
betatim
betatim reviewed Aug 30, 2023
View reviewed changes
betatim
betatim reviewed Aug 30, 2023
View reviewed changes
betatim
betatim reviewed Aug 30, 2023
View reviewed changes
betatim
betatim approved these changes Sep 5, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

Looks good to me.

WDYT @ogrisel

ogrisel
ogrisel approved these changes Sep 5, 2023
View reviewed changes
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 think we could improve the generic metric tests to include testing for sample_weight when applicable.

Other than that, LGTM.

@betatim
Copy link
Copy Markdown
Member

betatim commented Sep 7, 2023

I like the sample_weight suggestion, but also fine doing that in a new PR. @EdAbati what is your feeling/time availability?

@ogrisel ogrisel merged commit 844b087 into scikit-learn:main Sep 7, 2023
@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Sep 7, 2023

I merged, let's do that in a follow up PR :)

@ogrisel ogrisel mentioned this pull request Sep 7, 2023
Closed
@EdAbati
Copy link
Copy Markdown
Contributor Author

EdAbati commented Sep 7, 2023 •
edited
Loading

Hi both, sorry for the late reply I am a bit busy these days. I will likely make a follow-up PR during the weekend (if it is not done by then)

Thank you for merging :)

@EdAbati EdAbati deleted the zero-one-loss-array-api branch September 7, 2023 14:53
@EdAbati EdAbati mentioned this pull request Sep 11, 2023
Merged
@betatim betatim mentioned this pull request Oct 24, 2023
Open
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@EdAbati @REDVM
ENH Add Array API compatibility to zero_one_loss and `accuracy_scor…
2a7a597 
…e` (scikit-learn#27137)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OmarManzoor OmarManzoor OmarManzoor left review comments

@ogrisel ogrisel ogrisel approved these changes

@betatim betatim betatim approved these changes

Assignees

No one assigned

Labels

Array API module:metrics module:utils

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@EdAbati @OmarManzoor @ogrisel @betatim