Skip to content

[MRG+2] ENH labels parameter in P/R/F may extend or reduce label set #4287

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 0 commits into from
Jun 8, 2015

Conversation

jnothman
Copy link
Member

This PR replaces #2610, making the labels parameter to precision_recall_fscore_support more functional (and better documented, tested), but in accordance with #4192 not deprecating pos_label, but instead being restricted to the average != 'binary' case.

A common use of the micro-average is to extend the notion of binary P/R/F to the case where there is a frequent "negative class" and multiple classes of interest. Following this PR, explicitly listing the labels of interest allows the negative class to be excluded from a multiclass problem. (The same result can be achieved by transforming a multiclass problem into a multilabel problem excluding one label, but in the model evaluation API that would necessitate a custom and tricky scoring object.)

@jnothman
Copy link
Member Author

jnothman commented Mar 1, 2015

Ahh it seems rebase incorporated a failing test due to #4147. See https://github.com/scikit-learn/scikit-learn/pull/4147/files#r25564051

@jnothman
Copy link
Member Author

jnothman commented Mar 1, 2015

I've changed that test, hopefully not weakening it substantially in the multiclass case.

@jnothman
Copy link
Member Author

(Travis had failed due to a heisenbug and I hadn't noticed for a while...)

@jnothman
Copy link
Member Author

Rebased.

@arjoly
Copy link
Member

arjoly commented Apr 14, 2015

An unfortunate amount of code here is dedicated to continued support of the deprecated multilabel sequence of sequences format. I'll be glad to see it go.

This is now deprecated. We can dig that out of the codebase.

@jnothman
Copy link
Member Author

This is now deprecated. We can dig that out of the codebase.

Thanks for the reminder. I've cleared this out, I think.

@@ -498,8 +498,13 @@ def f1_score(y_true, y_pred, labels=None, pos_label=1, average='binary',
y_pred : 1d array-like, or label indicator array / sparse matrix
Estimated targets as returned by a classifier.

labels : array
Integer array of labels.
labels : array-like, optional
Copy link
Member

Choose a reason for hiding this comment

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

It's more a list-like, isn't it?

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 don't think "list-like" is used elsewhere in the codebase for lists of labels/classes... I'm fine for this to be list.

@jnothman
Copy link
Member Author

jnothman commented Jun 6, 2015

Rebased

@jnothman
Copy link
Member Author

jnothman commented Jun 6, 2015

Rebased and Gaël's doc comment addressed

@@ -315,7 +382,7 @@ def test_precision_refcall_f1_score_multilabel_unordered_labels():
y_pred = np.array([[0, 0, 1, 1]])
for average in ['samples', 'micro', 'macro', 'weighted', None]:
p, r, f, s = precision_recall_fscore_support(
y_true, y_pred, labels=[4, 1, 2, 3], warn_for=[], average=average)
Copy link
Member

Choose a reason for hiding this comment

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

huh the previous test seems odd...

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed

@amueller
Copy link
Member

amueller commented Jun 6, 2015

LGTM apart from minor comments.

@jnothman
Copy link
Member Author

jnothman commented Jun 6, 2015

Thanks for the review @amueller!

@jnothman jnothman changed the title [MRG] ENH labels parameter in P/R/F may extend or reduce label set [MRG+1] ENH labels parameter in P/R/F may extend or reduce label set Jun 6, 2015
excluded, for example to calculate a multiclass average ignoring a
majority negative class, while labels not present in the data will
result in 0 components in a macro average. By default, all labels in
``y_true`` and ``y_pred`` are used in sorted order.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment about the labels in multi-label classification?

Otherwise thanks, the docstring is a lot better!

@arjoly
Copy link
Member

arjoly commented Jun 8, 2015

Can you add a test ensuring that specifying labels with more labels than y_true / y_pred in multilabel classificaiton raises an error?

edit: I mean for micro, weighted and sample averaging as macro is already covered by your test.

assert_array_equal([.5, 1.], recall_13(average=None))
assert_equal((.5 + 1.) / 2, recall_13(average='macro'))
assert_equal((.5 * 2 + 1. * 1) / 3, recall_13(average='weighted'))
assert_equal(2. / 3, recall_13(average='micro'))
Copy link
Member

Choose a reason for hiding this comment

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

assert_array_almost_equal and assert_almost_equal?

@arjoly
Copy link
Member

arjoly commented Jun 8, 2015

Do we still support the custom label order?

@jnothman
Copy link
Member Author

jnothman commented Jun 8, 2015

Thanks for the review. Is there a reason we shouldn't still support custom
label order? Docstring says "... their order if average is None".

Also not sure what you want me to say about multilabel in the document
given "Labels present in the data can be excluded" is already said. I don't
know of use-cases for multilabel data where labels is useful in a
non-diagnostic scoring setting. Can you think of one?

On 8 June 2015 at 18:06, Arnaud Joly notifications@github.com wrote:

Do we still support the custom label order?


Reply to this email directly or view it on GitHub
#4287 (comment)
.

@arjoly
Copy link
Member

arjoly commented Jun 8, 2015

I was unclear. Could you specify what you should be specified if multilabel data is passed? I would add something like : "In multi-label classification, labels are the column indices."

@jnothman
Copy link
Member Author

jnothman commented Jun 8, 2015

Can you add a test ensuring that specifying labels with more labels than y_true / y_pred in multilabel classificaiton raises an error?
edit: I mean for micro, weighted and sample averaging as macro is already covered by your test.

You realise that for micro, weighted and sample averaging, the result would be identical were labels added with no instances?

@jnothman
Copy link
Member Author

jnothman commented Jun 8, 2015

Your comments have been addressed, @arjoly. Thanks

On 8 June 2015 at 19:24, Arnaud Joly notifications@github.com wrote:

I was unclear. Could you specify what you should be specified if
multilabel data is passed? I would add something like : "In multi-label
classification, labels are the column indices."


Reply to this email directly or view it on GitHub
#4287 (comment)
.

@jnothman
Copy link
Member Author

jnothman commented Jun 8, 2015

Travis error appears unrelated.

@arjoly
Copy link
Member

arjoly commented Jun 8, 2015

Can you add a test ensuring that specifying labels with more labels than y_true / y_pred in multilabel classificaiton raises an error?
edit: I mean for micro, weighted and sample averaging as macro is already covered by your test.
You realise that for micro, weighted and sample averaging, the result would be identical were labels added with no instances?

Yes, however I would expect that all labels are specified given the indicator matrix encoding.

@arjoly
Copy link
Member

arjoly commented Jun 8, 2015

LGTM !

Thanks @jnothman !

@arjoly arjoly changed the title [MRG+1] ENH labels parameter in P/R/F may extend or reduce label set [MRG+2] ENH labels parameter in P/R/F may extend or reduce label set Jun 8, 2015
@jnothman
Copy link
Member Author

jnothman commented Jun 8, 2015

throwing in what's new.

@jnothman
Copy link
Member Author

jnothman commented Jun 8, 2015

and squashed

@jnothman
Copy link
Member Author

jnothman commented Jun 8, 2015

shall i merge, then?

@arjoly
Copy link
Member

arjoly commented Jun 8, 2015

You can !! Thanks @jnothman

@jnothman jnothman closed this Jun 8, 2015
@jnothman jnothman merged commit 019fc9b into scikit-learn:master Jun 8, 2015
@arjoly
Copy link
Member

arjoly commented Jun 8, 2015

Thanks @jnothman !

@amueller
Copy link
Member

amueller commented Jun 8, 2015

Thanks!
Btw, there are currently no known travis errors, I think. If you find an unrelated error, please ping me.

@jnothman
Copy link
Member Author

jnothman commented Jun 9, 2015

You're welcome; I'm glad to have this support a meaningful multiclass micro-average.

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