-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
ENH Add zero division handling to cohen_kappa_score #31172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
ENH Add zero division handling to cohen_kappa_score #31172
Conversation
sklearn/metrics/_classification.py
Outdated
msg = ( | ||
"`y2` does not contain any label that is also both present in `y1` and in " | ||
"`labels`. cohen_kappa_score is undefined and set to the value defined in " | ||
"the `replace_undefined_by` param, which defaults to 0.0." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my thinking, this warning message also covers cases where both y1 and y2 are empty (as in "test case: empty inputs"). At least it would motivate people to inspect their data and then they might find out if their data is empty.
However, we could also branch here and make a separate warning message for empty inputs.
"`y1` and `y2` only have one label in common that is also in `labels`. " | ||
"cohen_kappa_score is undefined and set to the value defined in the " | ||
"`replace_undefined_by` param, which defaults to 0.0." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my thinking, this message also fits the cases when y1 and y2 only deal with one label (as in "test case: both inputs only have one label").
mgs_changing_default = ( | ||
"The default return value of `cohen_kappa_score` in case of a division " | ||
"by zero has been deprecated in 1.7 and will be changed to 0.0 in version " | ||
"1.9. Set `replace_undefined_by=0.0` to use the new default and to silence " | ||
"this Warning." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would pick 0.0 as a future default (instead of -1.0 which is the worst score), because it is the least expressive of the scores, representing matching labels by chance.
If users would use cohen_kappa_score
as part of their custom metric, that calculates the mean over several cohen_kappa_scores, 0.0 would be a neutral element like the "ignore" option that we have talked about in this comment: #29048 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @StefanieSenger!
Overall, I think the replace_undefined_by parameter
behaves as expected, as discussed in #29048 (comment).
I just have a few minor suggestions—otherwise, LGTM!
@pytest.mark.parametrize("replace_undefined_by", [0.0, np.nan]) | ||
def test_cohen_kappa_zero_division(replace_undefined_by): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test function looks good to me overall—my comment is just a minor nitpick:
@pytest.mark.parametrize("replace_undefined_by", [0.0, np.nan]) | |
def test_cohen_kappa_zero_division(replace_undefined_by): | |
@pytest.mark.parametrize( | |
"test_case", | |
[ | |
([], [], None, None, None), | |
([1] * 5 + [2] * 5, [3] * 10, [1, 2], None, None), | |
([3] * 10, [3] * 10, None, None, None), | |
([1] * 5 + [2] * 5, [3] * 10, [1, 2], "linear", None), | |
], | |
) | |
@pytest.mark.parametrize("replace_undefined_by", [0.0, np.nan]) | |
def test_cohen_kappa_zero_division(replace_undefined_by): |
This way, we could also simplify the unpacking slightly:
y1, y2, labels, weights, sample_weight = test_case
y_1, y2 = np.array(y1), np.array(y2)
and only need one assert
:
assert check_equal(
cohen_kappa_score(
y1,
y2,
labels=labels,
weights=weights,
replace_undefined_by=replace_undefined_by,
),
replace_undefined_by,
)
Totally optional though—happy for you to resolve this if you’d prefer to keep it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good suggestion.
I have made this test leaner, I couldn't do without commenting every single test case though. 🤷 To help the next person looking at this see why these test cases trigger a division by zero.
Co-authored-by: Virgil Chan <virchan.math@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your review, @virchan!
That was pretty helpful and I have applied all your suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @StefanieSenger!
@adrinjalali, would you like to have a look?
Reference Issues/PRs
towards #29048
What does this implement/fix? Explain your changes.
This PR adds a
replace_undefined_by
param tocohen_kappa_score
to deal with cases of division by zero.Also adds tests.