Skip to content

Conversation

EdAbati
Copy link
Contributor

@EdAbati EdAbati commented Sep 11, 2023

Reference Issues/PRs

See comments in #27137

What does this implement/fix? Explain your changes.

As suggested in the review, this PR adds an additional test for all the Array API-compatible metrics that have a sample_weight argument.

cc @ogrisel @betatim

@github-actions
Copy link

github-actions bot commented Sep 11, 2023

✔️ Linting Passed

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

Generated for commit: 39074ef. Link to the linter CI: here

@ogrisel
Copy link
Member

ogrisel commented Sep 11, 2023

I think we can ignore the partial coverage of the newly added conditions on the codecov report:

https://app.codecov.io/gh/scikit-learn/scikit-learn/pull/27335/blob/sklearn/metrics/tests/test_common.py#L1763

It means that the case where the condition is False never holds, but this is fine and I think it's safer to write the code this way because I am pretty sure that not all metrics support sample weights.

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.

Thanks for the follow-up. Here is a small suggestion for improving readability / intent, but otherwise LGTM!

EdAbati and others added 2 commits September 11, 2023 10:18
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@EdAbati
Copy link
Contributor Author

EdAbati commented Oct 24, 2023

Hey @betatim , not sure if you saw this one. :) This PR is addressing the suggestion here: #27137 (comment)

@betatim
Copy link
Member

betatim commented Oct 24, 2023

Thanks for the ping, I had indeed missed this! Looking now

@betatim betatim merged commit 8f3136b into scikit-learn:main Oct 24, 2023
@betatim
Copy link
Member

betatim commented Oct 24, 2023

Merged!

@EdAbati EdAbati deleted the improve-array-api-metrics-tests branch October 24, 2023 12:20
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 31, 2023
…arn#27335)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
…arn#27335)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
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.

3 participants