Skip to content

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

StefanieSenger
Copy link
Contributor

Reference Issues/PRs

towards #29048

What does this implement/fix? Explain your changes.

This PR adds a replace_undefined_by param to cohen_kappa_score to deal with cases of division by zero.
Also adds tests.

Copy link

github-actions bot commented Apr 10, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 2ee10a3. Link to the linter CI: here

Comment on lines 894 to 898
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."
)
Copy link
Contributor Author

@StefanieSenger StefanieSenger Apr 10, 2025

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."
)
Copy link
Contributor Author

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").

@StefanieSenger StefanieSenger added this to the 1.7 milestone Apr 10, 2025
Comment on lines +903 to +908
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."
)
Copy link
Contributor Author

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)

Copy link
Member

@virchan virchan left a 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!

Comment on lines 929 to 930
@pytest.mark.parametrize("replace_undefined_by", [0.0, np.nan])
def test_cohen_kappa_zero_division(replace_undefined_by):
Copy link
Member

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:

Suggested change
@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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@StefanieSenger StefanieSenger left a 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.

Copy link
Member

@virchan virchan left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants