ENH check_scoring() has raise_exc for multimetric scoring
#28992
Conversation
|
If this is wanted, then we need to add a changelog entry. And since the CI "Check Changelog" did not alarm, should we move |
|
Thanks @adrinjalali, I have now added a test. |
sklearn/metrics/_scorer.py
Outdated
|
|
||
| raise_exc : bool, default=True | ||
| Whether to raise an exception if a subset of the scorers in multimetric scoring | ||
| fails or to return an error code. |
There was a problem hiding this comment.
There is something weird here maybe the "to" should not be here.
There was a problem hiding this comment.
Hm, I believe it's correct English. Whether to do A if condition or to do B.
There was a problem hiding this comment.
I see, do you mind inverting both part around the "or":
Whether to return an error code or to raise an ...
Having a really long sentence before the "or" make my parsing algorithm fail :).
There was a problem hiding this comment.
I can see what you mean.
However, I really think that with those kinds of speaking params (raise_exc) we should document what happens when True first, because this is the natural meaning (and the default). Otherwise we force people to switch signs while reading.
What about using parenthesis like so:
Whether to raise an exception (if a subset of the scorers in multimetric scoring fails) or to return an error code.
There was a problem hiding this comment.
The parenthesis make the trick for me.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
There was a problem hiding this comment.
Thank you @glemaitre. I have added an example, but had to exclude it from doctest. Would you mind having another look?
sklearn/metrics/_scorer.py
Outdated
|
|
||
| raise_exc : bool, default=True | ||
| Whether to raise an exception if a subset of the scorers in multimetric scoring | ||
| fails or to return an error code. |
There was a problem hiding this comment.
Hm, I believe it's correct English. Whether to do A if condition or to do B.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
| - a callable returning a dictionary where the keys are the metric | ||
| names and the values are the metric scorers; | ||
| - a dictionary with metric names as keys and callables a values. | ||
| - a list, tuple or set of unique strings; |
There was a problem hiding this comment.
should we replace list, tuple, or set to iterable of strings?
There was a problem hiding this comment.
Hm, I actually think we should tell the users explicitly what the can pass in. Any other than those wouldn't show the correct behaviour in li. 1054-56:
if isinstance(scoring, (list, tuple, set, dict)):
scorers = _check_multimetric_scoring(estimator, scoring=scoring)
return _MultimetricScorer(scorers=scorers, raise_exc=raise_exc)And, any other iterable of strings would raise during the @validate_params().
So I would rather leave it as it is.
Reference Issues/PRs
#28360
#28359
What does this implement/fix? Explain your changes.
This PR suggests to expose
raise_excincheck_scoringto facilitate a public API for multrimetric scoring.Thus
check_scoringcan also be used incross_validatein a simpler way (this is actually how I came across it).