FIX limit warnings for recall_score, precision_score, f1_score,

[jnothman]

fixes #2586

[coveralls]

Coverage remained the same when pulling ae4d0ac on jnothman:prf_derivative_warnings into a8089b0 on scikit-learn:master.

[arjoly]

Looks good to me.

[mblondel]

@arjoly @jnothman I'm not familiar with this area of the code so don't wait for my +1 to merge this...

[arjoly]

Why not using assert_warns and assert_no_warnings?

[jnothman]

Only because I was copying the test already there -- which was written before we had imported assert_warns -- and because I had no interest spending time fixing those up in this change.

[coveralls]

Coverage remained the same when pulling a23662d on jnothman:prf_derivative_warnings into af43c88 on scikit-learn:master.

[jnothman]

Actually, @arjoly, I forgot: I can and have now used assert_no_warnings here. However, assert_warns does not (currently) support checking the message, only the class, of the warning, and @ogrisel had suggested I test the messages since their construction is non-trivial.

If you're fine with this PR, @arjoly, perhaps it's trivial enough to merge with a single +1...

[arjoly]

It would be nice to have a second thought on the new warn_for argument.
Should it be private?

[jnothman]

Yes, I wondered that. What does it mean to be private? I can prefix it with
a '_', or we can make precision_recall_fscore_support, recall_score, etc.
call a new _precision_recall_fscore_support with all the arguments
passed... I decided to avoid that option because "flat is better than
nested".

On Tue, Nov 19, 2013 at 6:41 PM, Arnaud Joly notifications@github.comwrote:

It would be nice to have a second thought on the new warn_for argument.
Should it be private?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2592#issuecomment-28771322
.

[jnothman]

So what do you think, @arjoly, a parameter named _warn_for, or a function named _precision_recall_fscore_support?

[arjoly]

I think that I would go for your solution, but consider that warn_for is a public argument. As you said "flat is better than nested". And one might want to disable warning using the warn_for argument.

[arjoly]

@ogrisel, @GaelVaroquaux, @amueller Any thoughts on the warn_for arguments?

[arjoly]

I think that I would go for your solution, but consider that warn_for is a public argument. As you said "flat is better than nested". And one might want to disable warning using the warn_for argument.

Still think that this is the way to go with a mention that this is for internal use.

[arjoly]

Why it doesn't raise a ValueError here if metric is not in warn_for?

[jnothman]

That's what we're handling: 'precision' won't be in warn_for when we're calling recall_score. The output of this division will be ignored anyway (and indeed, we could avoid that further up stream).

[arjoly]

Thanks !

[ogrisel]

Why it doesn't raise a ValueError here if metric is not in warn_for?

This the way to disable warnings as you requested earlier isn't it?

[arjoly]

I was thinking to what happened when somebody put some garbage string in warn_for and thought that it might generate an error. But the current behaviour is also fine.

[ogrisel]

About the warn_for argument I think I am fine with leaving it like that. The docstring already mentions "for internal use".

[arjoly]

Ok then +1 for merge !!

[ogrisel]

+1 as well. I will push the green button.

[arjoly]

Thanks @jnothman

[jnothman]

Thanks! It is good to know @mblondel can now sleep easy at night.

[mblondel]

Thanks! It is good to know @mblondel can now sleep easy at night.

I reported a problem in the code base. Apparently, you were aware of it before merge but didn't bother to fix it so don't blame me for your poor standards. Also if you could stick to the facts, that would be nice. Thanks!

[jnothman]

Well, now I'm sorry for some offence you appear to have taken at a joke! I wasn't accusing you of anything, nor passing blame...