Skip to content

ENH Add Array API compatibility to zero_one_loss and accuracy_score - #27137

Merged
ogrisel merged 14 commits into
scikit-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 into
scikit-learn:mainfrom
EdAbati:zero-one-loss-array-api

Conversation

@EdAbati

@EdAbati EdAbati commented Aug 22, 2023

Copy link
Copy Markdown
Contributor

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

github-actions Bot commented Aug 22, 2023

Copy link
Copy Markdown

✔️ 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 left a comment

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.

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

Comment thread sklearn/metrics/tests/test_common.py Outdated
Comment thread sklearn/utils/tests/test_array_api.py Outdated
Comment thread sklearn/utils/tests/test_array_api.py
@OmarManzoor

Copy link
Copy Markdown
Contributor

@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

EdAbati commented Aug 30, 2023

Copy link
Copy Markdown
Contributor Author

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

ogrisel commented Aug 30, 2023

Copy link
Copy Markdown
Member

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

ogrisel commented Aug 30, 2023

Copy link
Copy Markdown
Member

Ok for moving _array_api_for_tests, as well.

Comment thread sklearn/utils/_array_api.py Outdated
Comment thread sklearn/utils/tests/test_array_api.py Outdated
Comment thread sklearn/utils/tests/test_array_api.py Outdated

@betatim betatim left a comment

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.

Looks good to me.

WDYT @ogrisel

@ogrisel ogrisel left a comment

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 think we could improve the generic metric tests to include testing for sample_weight when applicable.

Other than that, LGTM.

Comment thread sklearn/metrics/tests/test_common.py
Comment thread sklearn/metrics/tests/test_common.py
Comment thread sklearn/metrics/tests/test_common.py
Comment thread sklearn/metrics/tests/test_common.py
Comment thread sklearn/metrics/tests/test_common.py
@betatim

betatim commented Sep 7, 2023

Copy link
Copy Markdown
Member

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

ogrisel commented Sep 7, 2023

Copy link
Copy Markdown
Member

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

@EdAbati

EdAbati commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

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

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.

4 participants