Skip to content

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

Merged
merged 17 commits into from
Sep 29, 2020

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Aug 18, 2020

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

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]
Copy link
Member Author

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?

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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]
Copy link
Member

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?

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

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.

I don't really like the helper name but otherwise LGTM:

@glemaitre
Copy link
Member Author

I renamed the function.

@thomasjpfan
Copy link
Member

I renamed the function.

Were the changes pushed to this PR?

@cmarmo
Copy link
Contributor

cmarmo commented Sep 29, 2020

@glemaitre apparently the requested changes haven't been pushed: do you mind fixing this? Then this pull request should probably be merged. Thanks!

@glemaitre
Copy link
Member Author

Whoops, just pushed the missing commit.

@thomasjpfan thomasjpfan merged commit 96b86b6 into scikit-learn:master Sep 29, 2020
amrcode pushed a commit to amrcode/scikit-learn that referenced this pull request Oct 19, 2020
…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>
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
…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>
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