[MRG] API: Fix description of brier_score_loss #14123

stefan-matcovici · 2019-06-19T05:34:44Z

Reference Issues/PRs
Fixes #13887

What does this implement/fix? Explain your changes.
Introduced new scorer neg_brier_loss_score to reflect the real behavior of the scorer
Added deprecation of brier_loss_score (not sure about the versions mentioned in the deprecation message 😕)
Updated documentation to reflect changes

Any other comments?

… to log_loss

jnothman · 2019-06-19T22:33:31Z

This is not a doc change, but an API change. Please update the title and description

ernstklrb · 2019-06-25T14:03:02Z

I suggest the new negated metric method to be called neg_brier_score. The old function has 'loss' added to it's name to point to the fact that "lower is better". This is no longer necessary with the new method, 'loss' can be dropped.

jnothman · 2019-06-25T22:40:07Z

Well the convention we have elsewhere is to provide a smaller-is-better loss function but a greater-is-better negated scorer. This may be largely for historical reasons, but I think it would be wise to be consistent.

sklearn/metrics/classification.py

amueller · 2019-07-02T15:06:21Z

I think for the scorer having neg_bier_score is the right and consistent thing to do; seems like we overlooked this when doing the other deprecations. But we don't need to change the score function.

You're also adding a method to list the valid losses. I would prefer to the PR to do just one thing.

There's also another change that seems not related and not tested.

stefan-matcovici · 2019-07-10T12:38:19Z

I am open to any other solution of printing out the list of non-deprecated scorers. As currently this method is used just in get_scorer I can move the logic just there and actually print all the scorers in case of wrong scoring method parameter

…into pr/13918 # Conflicts: # sklearn/metrics/tests/test_score_objects.py

amueller · 2019-08-22T17:48:17Z

I'm fine with having the method, but I would make it private. otherwise looks good.

stefan-matcovici · 2019-08-22T18:48:12Z

Why private? In the end don't we want the user to be able to see what are the actual valid scorers?

amueller · 2019-08-22T18:55:40Z

Because I consider it an implementation detail and if we make anything public we need to add deprecation cycles if we ever want to change anything about it, and that's a pain.

stefan-matcovici · 2019-08-22T18:57:20Z

sklearn/metrics/scorer.py

@@ -229,8 +251,8 @@ def get_scorer(scoring):
            scorer = SCORERS[scoring]
        except KeyError:
            raise ValueError('%r is not a valid scoring value. '
-                             'Use sorted(sklearn.metrics.SCORERS.keys()) '
-                             'to get valid options.' % (scoring))
+                             'Use sorted(sklearn.metrics.valid_scorers()) '


But the message here should remain the same right?

hm you're right, that's a bit weird if we make it private.
We could just print the output of valid_scorers here. I think we removed this here at some point but I don't remember why.

#14123 (comment)

I can't remember either. Maybe I thought the list will be too long for an error message. Now, I believe that's the easiest way and I will get rid of the valid_scorers method too

Now I know why I decided to do that. We print the output of a wrong call in the documentation. https://github.com/scikit-learn/scikit-learn/pull/14123/files#diff-1a86c825aeb118f98447f3435891d362R112.
When adding a new scorer we should update the documentation every time which is not explicit at all. What do you think about going back to valid_scorers function?

qinhanmin2014

thanks for the PR and sorry for the slow reply, do you have time to finish this one? @stefan-matcovici

sklearn/metrics/scorer.py

sklearn/metrics/tests/test_score_objects.py

* DOC Link items explictly * STY * DOC Link another

stefan-matcovici · 2019-09-05T12:01:47Z

Yes, I have time, I will try to finish it today

qinhanmin2014 · 2019-09-05T12:41:52Z

Yes, I have time, I will try to finish it today

thanks

qinhanmin2014 · 2019-09-05T13:07:26Z

Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

- change neg_brier_score to neg_brier_score_loss

… to log_loss

This reverts commit 17a6886.

- change neg_brier_score to neg_brier_score_loss

…-learn into pr/13918 � Conflicts: � sklearn/metrics/tests/test_score_objects.py

qinhanmin2014 · 2019-09-06T00:28:38Z

@stefan-matcovici the diff looks strange, feel free to open another PR.

bharatr21 and others added 8 commits May 21, 2019 16:23

Fix description of brier_score_loss by mentioning negated behaviour

6c09714

Rename to neg_brier_score_loss and deprecate brier_score_loss similar…

7a86a04

… to log_loss

some changes to brier_score_loss

1ec0760

change way of showing deprecation

043a16f

add neg_brier_score_loss to common tests

1a8bbfd

fix pep8 indentation error

4f09ed5

fix error message causing unittests to fail

bd75bf1

change error message and add valid_scorers method

9e66f8c

stefan-matcovici changed the title ~~[MRG] DOC: Fix description of brier_score_loss~~ [MRG] API: Fix description of brier_score_loss Jun 20, 2019

add unittests

aa59910

stefan-matcovici added 2 commits June 26, 2019 20:46

remove unnecessary pass

9cb9ef8

change neg_brier_score_loss to neg_brier_score

be371e8

amueller reviewed Jul 2, 2019

View reviewed changes

sklearn/metrics/classification.py Show resolved Hide resolved

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

c52f9c7

…into pr/13918 # Conflicts: # sklearn/metrics/tests/test_score_objects.py

stefan-matcovici commented Aug 22, 2019

View reviewed changes

stefan-matcovici and others added 7 commits August 22, 2019 16:32

remove valid_scorers function

17a6886

TST mean_absolute_error multioutput test fix (scikit-learn#14733)

ecc0488

MAINT Uses latest version of python (scikit-learn#14722)

7ecd521

MAINT Fix assert raises in sklearn/svm/tests/ (scikit-learn#14727)

8e8e60e

DOC: Links to ONNX and PMML not correctly rendered

edcfa0d

ENH: Add direct CircleCI artifact link (scikit-learn#14731)

43dd2e8

DOC Cleans up roc curve plotting (scikit-learn#14723)

f09839d

Fix mixin inheritance order, allow overwriting tags (scikit-learn#14884)

03ea20d

qinhanmin2014 reviewed Sep 5, 2019

View reviewed changes

sklearn/metrics/scorer.py Outdated Show resolved Hide resolved

sklearn/metrics/scorer.py Outdated Show resolved Hide resolved

qinhanmin2014 approved these changes Sep 5, 2019

View reviewed changes

sklearn/metrics/tests/test_score_objects.py Outdated Show resolved Hide resolved

qinhanmin2014 added the Blocker label Sep 5, 2019

qinhanmin2014 added this to the 0.22 milestone Sep 5, 2019

DOC Link items explictly (scikit-learn#14817)

14f5302

* DOC Link items explictly * STY * DOC Link another

stefan-matcovici and others added 18 commits September 5, 2019 11:47

- change version of removing deprecated scorers

f51ed22

- change neg_brier_score to neg_brier_score_loss

Fix description of brier_score_loss by mentioning negated behaviour

205fef0

Rename to neg_brier_score_loss and deprecate brier_score_loss similar…

05ee6e3

… to log_loss

some changes to brier_score_loss

d3033a4

change way of showing deprecation

9b0c28f

add neg_brier_score_loss to common tests

2e3c2b5

fix pep8 indentation error

f4f71ac

fix error message causing unittests to fail

0dd58a1

change error message and add valid_scorers method

7dc8c6d

add unittests

4f2153e

remove unnecessary pass

d1b82e2

change neg_brier_score_loss to neg_brier_score

dc0275d

remove valid_scorers function

12074ac

Revert "remove valid_scorers function"

f7762c5

This reverts commit 17a6886.

- change version of removing deprecated scorers

372553b

- change neg_brier_score to neg_brier_score_loss

Merge branch 'pr/13918' of https://github.com/stefan-matcovici/scikit…

3b33db0

…-learn into pr/13918 � Conflicts: � sklearn/metrics/tests/test_score_objects.py

indentation fix

98f3db9

doc update

cede720

stefan-matcovici closed this Sep 6, 2019

stefan-matcovici mentioned this pull request Sep 6, 2019

[MRG] API Replace scorer brier_score_loss with neg_brier_score #14898

Merged

Uh oh!

[MRG] API: Fix description of brier_score_loss #14123

[MRG] API: Fix description of brier_score_loss #14123

Uh oh!

Conversation

stefan-matcovici commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnothman commented Jun 19, 2019

Uh oh!

ernstklrb commented Jun 25, 2019

Uh oh!

jnothman commented Jun 25, 2019 via email

Uh oh!

Uh oh!

amueller commented Jul 2, 2019

Uh oh!

stefan-matcovici commented Jul 10, 2019

Uh oh!

amueller commented Aug 22, 2019

Uh oh!

stefan-matcovici commented Aug 22, 2019

Uh oh!

amueller commented Aug 22, 2019

Uh oh!

stefan-matcovici Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

amueller Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

stefan-matcovici Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

stefan-matcovici Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefan-matcovici commented Sep 5, 2019

Uh oh!

qinhanmin2014 commented Sep 5, 2019

Uh oh!

qinhanmin2014 commented Sep 5, 2019

Uh oh!

qinhanmin2014 commented Sep 6, 2019

Uh oh!

Uh oh!

stefan-matcovici commented Jun 19, 2019 •

edited

Loading