-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MNT make error message consistent in brier_score_loss #18183
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
Conversation
if classes.dtype.kind not in ('O', 'U', 'S'): | ||
# for backward compatibility, if classes are not string then | ||
# `pos_label` will correspond to the greater label | ||
pos_label = classes[-1] |
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.
It is not super clear why we accept this use case?
@jnothman Do you have the keys for this one?
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.
Should we open an issue about this?
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 think we should and see if this is legit. I would not block this PR for this, however :)
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 think we should at least open an issue to discuss the possibility to deprecate this behavior this behavior.
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.
Personally I don't think it's legit. It's likely to cause hard to detect usage issues.
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.
Opened issue here: #18381
if classes.dtype.kind not in ('O', 'U', 'S'): | ||
# for backward compatibility, if classes are not string then | ||
# `pos_label` will correspond to the greater label | ||
pos_label = classes[-1] |
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.
Should we open an issue about this?
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
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
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 don't really like the helper name but otherwise LGTM:
I renamed the function. |
Were the changes pushed to this PR? |
@glemaitre apparently the requested changes haven't been pushed: do you mind fixing this? Then this pull request should probably be merged. Thanks! |
Whoops, just pushed the missing commit. |
…8183) * MNT make error message consistent in brier_score_loss * iter * TST change error matching * PEP8 * TST check consistent error message * STY * Apply suggestions from code review Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * iter * cleaner * iter * iter * iter * rename function with better name * iter Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
…8183) * MNT make error message consistent in brier_score_loss * iter * TST change error matching * PEP8 * TST check consistent error message * STY * Apply suggestions from code review Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * iter * cleaner * iter * iter * iter * rename function with better name * iter Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
closes #15412
Make error message consistent with other metrics when
pos_label
needs to be specified with string.Note that introducing the
_check_ambiguity_pos_label
function will be useful in #11096