Fine, but those are the wrong values because your precision and recall are swapped. I.e. in the indicator matrix test, we should have:

>>> p = np.array([1/3, 2/3, 1/3])
>>> r = np.array([1,1,1]
>>> (1.25 * p * r / (.25 * p + r)).mean()
0.494...

(not .779 which is (1.25 * p * r / (.25 * r + p)).mean())

From pr #1945, there is still one remaining question

• What should we do when the computation of the precision, recall or fscore lead to a nan ? Currently if the computation lead to a nan, the related measure is set to 0.0.

Yes, I didn't see that question there. I think logically that when the denominator for P or R is 0, the metric should be 1, but this is not what previous versions of scikit-learn did. For backwards consistency, you should implement the = 0 version in average='samples'. But I would like to see this changed, with others' approval.

Finally, if the numerator is 0 for F-score (no true positives), the metric should be 0. If the denominator is 0, the user shouldn't be calling the function.

In any case, I've rewritten the entire function, but not fixed the tests in #1990. Thanks for your hard work on the way to this cleaner implementation.

Yes, I didn't see that question there. I think logically that when the denominator for P or R is 0, the metric should be 1, but this is not what previous versions of scikit-learn did. For backwards consistency, you should implement the = 0 version in average='samples'. But I would like to see this changed, with others' approval.

I would keep the implementation as it is - i.e., return 0 if the denominator is 0. In addition, we could trigger a warning when such a case occurs since the value is actually undefined.

Note that this is what Weka does as well, it returns 0 in such cases.

Just my 2 cents.

In Mulan, the multilabel extension of weka, the precision is set to 1 if (tp + fp+ fn == 0),
to 0 if (tp + fp == 0) and compute the precision otherwise.

At the moment, I will stick with default current behavior.

@jnothman Note that we have called average="samples" correspond to their example based metrics.

I would keep the implementation as it is - i.e., return 0 if the
denominator is 0. In addition, we could trigger a warning when such a
case occurs since the value is actually undefined.

+1 and +1 for the warning.

+1 for the warning

Would "example" be better than "samples" for consistency with the literature? I suggested "samples" because this seems to be the term in common sklearn use for the first axis, where it might otherwise be "examples" or "instances".

A warning's a good idea. But actually, I think recall with 0 true samples is undefined, but at least in the context of precision-recall curves, precision with 0 predicted samples (or above the threshold where any samples are recalled) should be 1.

Now warnings are raised. However, those are not perfect. Undefined recall could raise a warning when precision_score is called. This can't be avoided due the coupling in computation of the precision, recall and fscore in the current implementation.

I have also removed the pos_label argument with the binary indicator format.

A note on the fact that micro-precision, micro-f1 and micro-recall might not be equal is needed.

A note on the fact that micro-precision, micro-f1 and micro-recall might not be equal is needed.

You mean the contrary? A note that micro-PRF is equal in certain cases? I don't actually think this belongs in the docstring, but in the narrative doc when the advantages and disadvantages of each metric will hopefully be described. Certainly, it's not related to this PR.

I don't understand why the warning is not raised at the moment. :-(

Otherwise, I think that I have taken into account all your remarks @jnothman.

I don't understand why the warning is not raised at the moment. :-(

It's only failing for 'samples'... but I also can't understand why.

Note that if you run the test by itself, there's no error:

$ nosetests sklearn/metrics/tests/test_metrics.py:test_precision_recall_f1_no_labels

This suggests the warning not being caught is due to interaction with other times that warning has been called. Perhaps assert_warns (and its warnings.simplefilter(...)) is not working very well. Maybe you're best off reverting the porting of assert_warns (sorry! I only wish this was easier to test.).

And when tests are passing you might want to change the title of this PR from WIP to MRG... As a bugfix, I think we want it in master soon.

In particular, the test fails only if run after one of two other tests:

$ tests=$(grep '^def test_' sklearn/metrics/tests/test_metrics.py | sed 's/def //;s/(.*)://')
$ for f in $tests
> do
>   nosetests sklearn/metrics/tests/test_metrics.py:$f \
>             sklearn/metrics/tests/test_metrics.py:test_precision_recall_f1_no_labels \
>             2>/dev/null ||
>   echo $f
> done
test_multilabel_representation_invariance
test_multilabel_invariance_with_pos_labels

It may not be about the warning filter failing, as the following runs without a problem:

$ nosetests sklearn/metrics/tests/test_metrics.py:test_precision_recall_f1_no_labels sklearn/metrics/tests/test_metrics.py:test_precision_recall_f1_no_labels

Right. Apparently the solution is to stick warnings.simplefilter('always') inside the catch_warnings context (at least in those two tests). I don't understand why this is necessary, seeing as assert_warns creates its own context in which it calls simplefilter('always'). Surely that should mean it is independent of prior filter settings and the record of previously-seen messages...?

I should stop dwelling on this, but I think the right way to handle warnings in tests is:

• have a separate test to check that a warning is raised in certain cases, in which you first assert that no warning is raised in a valid variant, then assert that one is raised in a problematic variant;
• tests that are known to raise warnings as a side-effect, but that's not the purpose of the test, should have a decorator available like @ignore_warnings([DeprecationWarning, UserWarning]);
• deprecation warnings should probably be tested in a separate test module.

Testing warnings is messy. I have drafted a few notes on a more ideal solution, but the current problem with this PR is due to a known Python bug.

They might belong to another PR, but #2094 reminded me of a couple of issues that should be tested with relation to binary data (not that they address #2094 directly):

• ensure that if y_true and y_pred include a single class label, it is treated as binary and pos_label is observed (where average is not None). (Perhaps this is already being tested.)
• ensure that if labels is specified and has more than two entries, it is treated as multiclass.

Warning bugs is fixed !!! Thanks @jnothman .

I switch this pr in MRG state.

About warnings (I am replying to old comments by @jnothman in this thread): we might consider implementing our own warnings, and setting them to be always raised, and not only once. In sklearn.utils.init there is currently a hack that forces DeprecationWarning to be always raised.

The code looks good. With regards to the maths, I will have to trust you on the changes, and to hope that the tests are good, as I haven't done the maths.

LGTM, but this is somewhat outside the feature set I use, so before
merging it would be good to have a second point of view.

Awesome ! Thanks for the review !

Oh dear. Travis says that Python warning bug strikes again; all catch_warnings uses in tests need an always filter! It's a good reason to use something like assert_warns.

Btw, for whoever else reviews this if another is needed (@glouppe?), the focus should be on the tests, as the implementation is likely to change soon.

It can go within any warning context. If you mean to switch it off for all tests: you can write a setup function for nosetests that sets up a warning context. It's not the most pleasant of code:

def setup():
    global warning_ctx
    warning_ctx = warnings.catch_warnings()
    warning_ctx.__enter__()
    warnings.simplefilter('ignore', UndefinedPRFWarning)

def teardown():
    global warning_ctx
    warning_ctx.__exit__()

@arjoly I know the goal of the PR, but among the bugs was some disagreement on how to handle this case, and we seem to have agreed that warnings are the solution.

no i wanted it to switch it off for all users ;)

Ok let's live with it. I removed those commits.

@amueller you mean you don't think there should be a warning?

@jnothman I'm not sure but warning too much tends to be just annoying.

I agree that warnings can be annoying, and they're particularly annoying in test output or when in a module you've not called directly. Hence they can be easily disabled, and we could do so by default with a warnings filter in the metrics module.

But sometimes they're also important, such as to suggest the user should have chosen a different setting for their input. In these cases we don't expect the warning to be frequent, and if the user knows what they're doing they can disable it.

I think the default warnings setting shows each warning once per execution; however, this is once per identical (class, message, triggering module/line). Frustratingly, this means warning messages need to be non-specific.

In sum:

• use warnings where they indicate the user may have passed a bad data/settings combination
• avoid detail in the message so filtering works
• if you think users will wish to silence that warning in particular, use a custom subclass of UserWarning
• ideally, warnings that are functional requirements should be tested, but in practice this can be painful

should we merge this now?

could you please rebase / merge? I guess your example removals caused merge problems.

As far as I'm concerned, it's good to merge.

@amueller I rebase on top of master

merged :) thanks a lot!

Cool !!! :-) Finally merged thanks to all reviewers !!!