Skip to content

[MRG + 1] Complete deprecation of default average in P/R/F metrics#7304

Merged
ogrisel merged 4 commits intoscikit-learn:masterfrom
jnothman:binary_prf_mine
Sep 8, 2016
Merged

[MRG + 1] Complete deprecation of default average in P/R/F metrics#7304
ogrisel merged 4 commits intoscikit-learn:masterfrom
jnothman:binary_prf_mine

Conversation

@jnothman
Copy link
Copy Markdown
Member

@jnothman jnothman commented Aug 31, 2016

Supersedes #7134 (and fixes prematurely-closed #5434).

Old behaviour:

  • average defaults to 'weighted'. This was nonstandard.
  • As long as average is not None and pos_label (default 1) is not None, having binary data meant binary P/R/F. This was brittle.
  • labels could not be used to limit the label set when averaging. This was lacking.

Deprecation behaviour:

  • average defaults to 'binary' but with the old semantics
  • labels can be used to limit the label set when averaging.
  • Warnings warnings everywhere

New behaviour:

  • average defaults to 'binary'.
  • average == 'binary' with non-binary data will result in an error so that the user chooses an explicit averaging mode.
  • average != 'binary' with binary data will result in averaging over each class in labels.
  • pos_label does nothing when average != 'binary'. If pos_label is not the default, and not None (due to legacy behaviour), we now issue a warning saying it's unused.

Also update tests for new behaviour and ensure there are warnings for some changed and confusing (pity) behaviours.
y_pred)


@ignore_warnings
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because without it there are warnings related to 0 denominators.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please add a comment to make that explicit?

@jnothman
Copy link
Copy Markdown
Member Author

We could allow the user to set labels=[pos_label], average='binary' in the multiclass/mulitlabel data case, but then should really raise ValueError("we're confused") if pos_label is not None and labels != [pos_label].

I had hoped to get rid of pos_label (really neg_label makes more sense here, but labels works fine too). We may yet. The above behaviour would be a step towards that. WDYT?

@amueller
Copy link
Copy Markdown
Member

amueller commented Sep 1, 2016

I would also hope to get rid of that but that makes it harder / less obvious to do The Right Thing (tm) in the binary case. We could also say "if average='binary' then pos_label is the smaller label and if you want anything else use the labels parameter"

@jnothman
Copy link
Copy Markdown
Member Author

jnothman commented Sep 1, 2016

THe problem with that strategy is that pos_label defaults to 1, and we need to throw an error if average='binary' and the user provides multiclass/multilabel data. Of that I'm adamant.

@jnothman
Copy link
Copy Markdown
Member Author

jnothman commented Sep 1, 2016

Getting rid of pos_label is easy enough, except for how long it's been around... and then there are all these other metrics with pos_label.

method over binary targets.
The class to report if ``average='binary'`` and the data is binary.
If the data are multiclass or multilabel, this will be ignored;
setting `labels=[pos_label]` and `average != 'binary'` will report
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Double quote for labels=[pos_label] and average != 'binary'

@amueller
Copy link
Copy Markdown
Member

amueller commented Sep 6, 2016

lgtm

@amueller amueller changed the title [MRG] Complete deprecation of default average in P/R/F metrics [MRG + 1] Complete deprecation of default average in P/R/F metrics Sep 6, 2016
@tguillemot
Copy link
Copy Markdown
Contributor

LGTM too :)

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Sep 8, 2016

LGTM as well. Squash-merging.

@ogrisel ogrisel merged commit 2bf96df into scikit-learn:master Sep 8, 2016
@raghavrv
Copy link
Copy Markdown
Member

raghavrv commented Sep 9, 2016

Thanks a lot @jnothman!

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.

Remove deprecated stuff that will no longer be supported by 0.18

5 participants