FIX Raise on empty inputs in accuracy_score #31187
Conversation
sklearn/metrics/_classification.py
Outdated
| thus ill-defined. Can take the following values: | ||
| - `np.nan` to return `np.nan` | ||
| - a floating point value in the range of [0.0, 1.0] or int 0 |
There was a problem hiding this comment.
| - a floating point value in the range of [0.0, 1.0] or int 0 | |
| - a floating point value in the range of $[0.0, 1.0]$ or `int` 0 |
I think that makes it math like
There was a problem hiding this comment.
Guillaume once told me we don't use latex in docstrings. So I will use back ticks instead.
sklearn/metrics/_classification.py
Outdated
| "defaults to 0 when `normalize=False` is set." | ||
| ) | ||
| warnings.warn(msg, UndefinedMetricWarning, stacklevel=2) | ||
| return replace_undefined_by if math.isnan(replace_undefined_by) else 0 |
There was a problem hiding this comment.
| return replace_undefined_by if math.isnan(replace_undefined_by) else 0 | |
| return replace_undefined_by if np.isnan(replace_undefined_by) else 0 |
since we're using np.nan?
There was a problem hiding this comment.
Both is possible, but we're replacing np.isnan with math.isnan for array api, so we can already do it here, I think.
| @@ -3081,7 +3114,7 @@ def hamming_loss(y_true, y_pred, *, sample_weight=None): | |||
| Returns | |||
| ------- | |||
| loss : float or int | |||
| loss : float | |||
There was a problem hiding this comment.
there return types are changed. I agree it should be always float for all of them, but it'd be nice to have a test for all the cases to make sure it's actually float.
There was a problem hiding this comment.
I will do this in a separate PR.
These tests had already been added in #30575.
There was a problem hiding this comment.
Thank you for your review, @adrinjalali.
I have addressed all your comments. For testing if all the classification metrics indeed return floats, I will open a separate PR.
sklearn/metrics/_classification.py
Outdated
| thus ill-defined. Can take the following values: | ||
| - `np.nan` to return `np.nan` | ||
| - a floating point value in the range of [0.0, 1.0] or int 0 |
There was a problem hiding this comment.
Guillaume once told me we don't use latex in docstrings. So I will use back ticks instead.
sklearn/metrics/_classification.py
Outdated
| "defaults to 0 when `normalize=False` is set." | ||
| ) | ||
| warnings.warn(msg, UndefinedMetricWarning, stacklevel=2) | ||
| return replace_undefined_by if math.isnan(replace_undefined_by) else 0 |
There was a problem hiding this comment.
Both is possible, but we're replacing np.isnan with math.isnan for array api, so we can already do it here, I think.
| @@ -3081,7 +3114,7 @@ def hamming_loss(y_true, y_pred, *, sample_weight=None): | |||
| Returns | |||
| ------- | |||
| loss : float or int | |||
| loss : float | |||
There was a problem hiding this comment.
I will do this in a separate PR.
These tests had already been added in #30575.
…scikit-learn into undefined_accuracy_score
| @@ -360,6 +376,18 @@ def accuracy_score(y_true, y_pred, *, normalize=True, sample_weight=None): | |||
| y_type, y_true, y_pred = _check_targets(y_true, y_pred) | |||
| check_consistent_length(y_true, y_pred, sample_weight) | |||
|
|
|||
| if _num_samples(y_true) == 0: | |||
There was a problem hiding this comment.
As discussed during the dedicated meeting, we believe that the fact that accuracy_score does not raise ValueError when called on empty arrays was an oversight:
- scikit-learn estimators always raise
ValueErrorwhen fit on empty data, - more importantly, they always raise
ValueErrorwhen predicting or transforming empty data, so it's not possible to get a working pipeline that would output an emptyy_predarray; - many other metric functions such as
roc_auc,f1_scoreandmean_squared_erroralready raiseValueError.
I think we can treat the accuracy_score case as a bug and make it raise ValueError instead (without deprecation cycle).
There was a problem hiding this comment.
All other metric functions call check_array or a similar function that rejects empty arrays.
This is apparently not the case for _check_targets used in accuracy_score. We should review the list of calls to _check_targets to check whether we should change this function to:
- reject empty arrays;
- maybe even call
check_arraydirectly, but we would then need to check for redundant input validation to avoid introducing unnecessary overhead of redundant costly checks.
|
I remove the 1.7 milestone for this one because there is no emergency to fix the bug described above for 1.7. |
|
Just to be sure: raising a ValueError on empty inputs also means that |
Reference Issues/PRs
towards #29048
What does this implement/fix? Explain your changes.
This PR adds a
replace_undefined_byparam toaccuracy_scoreto deal with empty y_true and y_pred.Also adds tests.
Open Question
Note that before this PR
accuracy_scorereturned like this:accuracy_score(np.array([]), np.array([]))accuracy_score(np.array([]), np.array([]), normalize=False)I would like to consider this inconsistency as a bug and fix this with this PR for the next release without deprecation, so it comes faster. Would this be okay? How would you see that, @adrinjalali?