ENH check_scoring() has raise_exc for multimetric scoring#28992
ENH check_scoring() has raise_exc for multimetric scoring#28992adrinjalali merged 12 commits intoscikit-learn:mainfrom
check_scoring() has raise_exc for multimetric scoring#28992Conversation
|
If this is wanted, then we need to add a changelog entry. And since the CI "Check Changelog" did not alarm, should we move |
adrinjalali
left a comment
There was a problem hiding this comment.
This would need a test. And since it's touching public API, it needs a changelog. Otherwise LGTM.
|
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.
glemaitre
left a comment
There was a problem hiding this comment.
It looks good apart from little nitpicks.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
StefanieSenger
left a comment
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.
glemaitre
left a comment
There was a problem hiding this comment.
With the two suggestions, I think that it will be enough.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
adrinjalali
left a comment
There was a problem hiding this comment.
minor nit, otherwise LGTM.
| - 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).