-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
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
MAINT Parameters validation for metrics.top_k_accuracy_score #25828
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job!
sklearn/metrics/_ranking.py
Outdated
{ | ||
"y_true": ["array-like"], | ||
"y_score": ["array-like"], | ||
"k": [Integral], |
There was a problem hiding this comment.
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],
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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!
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks
…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>
Towards #24862
metrics.top_k_accuracy_score
metrics.top_k_accuracy_score
intest_public_functions.py
Note: the
k
parameter also works for integersk <= 0
, which may not be its intended use case.