Skip to content

MAINT Parameters validation for metrics.top_k_accuracy_score #25828

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.

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

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

sortofamudkip
Copy link
Contributor

Towards #24862

  • Added parameter validation for metrics.top_k_accuracy_score
  • Added test for metrics.top_k_accuracy_score in test_public_functions.py

Note: the k parameter also works for integers k <= 0, which may not be its intended use case.

Copy link
Contributor

@Veghit Veghit left a comment

Choose a reason for hiding this comment

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

great job!

{
"y_true": ["array-like"],
"y_score": ["array-like"],
"k": [Integral],
Copy link
Contributor

Choose a reason for hiding this comment

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

why not limit usage to >1
i.e. "k": [Interval(Integral, 1, None, closed="left"), None],

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, k cannot be negative. We should use an interval here. However, we cannot use None because we expect only integral values.

Copy link
Member

Choose a reason for hiding this comment

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

None is fine since closed="left"

Copy link
Member

Choose a reason for hiding this comment

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

Ups, I misread. I read that it was an option None (not inside the interval). My bad.

Copy link
Member

Choose a reason for hiding this comment

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

Ah no you're right. I did misread and didn't see the additional None outside the interval :)

Copy link
Member

Choose a reason for hiding this comment

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

So I misread your misreading :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for all the suggestions and help!

@glemaitre glemaitre added No Changelog Needed Validation related to input validation labels Mar 13, 2023
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

I pushed the requested change. LGTM. Thanks @sortofamudkip

@glemaitre glemaitre enabled auto-merge (squash) March 13, 2023 14:05
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

@glemaitre glemaitre merged commit b0b354c into scikit-learn:main Mar 13, 2023
Veghit pushed a commit to Veghit/scikit-learn that referenced this pull request Apr 15, 2023
…learn#25828)

Co-authored-by: wishyut.pitawanik <wpitawanik@mpiwg-berlin.mpg.de>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants