[MRG] Fixed brier_score_loss function when y_true only has one label #8459
Conversation
…s one label by not letting label_binarize convert 1s to 0s.
Codecov Report
@@ Coverage Diff @@
## master #8459 +/- ##
==========================================
- Coverage 95.47% 95.47% -0.01%
==========================================
Files 342 342
Lines 60907 60911 +4
==========================================
+ Hits 58154 58157 +3
- Misses 2753 2754 +1
Continue to review full report at Codecov.
|
|
Hello, I'm seeing that code coverage was reduced slightly with this fix. The reason I made the pass-through conditional is because I see 3 approaches here:
Let me know which approach you prefer. |
| @@ -1463,5 +1463,7 @@ def test_brier_score_loss(): | |||
| assert_raises(ValueError, brier_score_loss, y_true, y_pred + 1.) | |||
| assert_raises(ValueError, brier_score_loss, y_true, y_pred - 1.) | |||
| # calculate even if only single class in y_true (#6980) | |||
| assert_almost_equal(brier_score_loss([0], [0.5]), 0.25) | |||
There was a problem hiding this comment.
sorry if i'm being slow, but is there any reason to remove these?
There was a problem hiding this comment.
@nelson-liu Considering the other tests that I had added, I felt these were redundant. I can add them back if you want.
There was a problem hiding this comment.
@qinhanmin2014 could you double check that this is the right solution?
|
@jnothman |
|
@qinhanmin2014 wrote an age ago:
That's not actually true, since y_true is binarized in We should add tests with pos_label=1 where negative is represented as -1, and pos_label='yes' when negative is represented as 'no', just to be sure. Otherwise this LGTM |
Sigh, I feel that again I wasted a whole year. |
|
What do you mean by changing the definition but also deprecating? I would
be happy to just deprecate the current behaviour towards doing the same as
ROC
|
I mean first change the definition of the default in 0.21 (this will be a small PR which might be easier to review), then deprecate pos_label=None in 0.22. |
|
I don't know if we should deprecate pos_label=None, but we could warn
and then raise an error if y_true is constant...?
…On Thu, 11 Apr 2019 at 11:30, Hanmin Qin ***@***.***> wrote:
What do you mean by changing the definition but also deprecating? I would
be happy to just deprecate the current behaviour towards doing the same as
ROC
I mean first change the definition of the default in 0.21 (this will be a small PR which might be easier to review), then deprecate pos_label=None in 0.22.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I'm aware that this might be controversial, this is why I propose to do it in another PR.
Why? constant y_true in still a valid option. I'll open a PR for you to review tomorrow evening. |
For issue #6980 a fix was introduced allowing the brier_score_loss function to accept y_true with a single label. Test cases were also added, but only the scenario where the probability is 0.5 was tested. 0.5 is problematic because it produces the same Brier score, regardless of whether the outcome is 0 or 1, and it's producing incorrect scores for values other than 0.5 in this scenario. Since brier_score_loss indirectly calls
label_binarize, y_true with a single label is converted to a zero-valued array, no matter what is set as a positive label:brier_score_loss([1], [0.4], pos_label=1)produces 0.16, because[1]is converted to[0]. The correct answer should have been 0.36.The fix contains a small modification that allows binary arrays to pass through (so it skips label_binarize). Test cases have also been added.
Let me know if anything else is required.