ENH Add zero division handling to cohen_kappa_score

[StefanieSenger]

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.

[github-actions]

✔️ Linting Passed

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

_{Generated for commit: ed62103. Link to the linter CI: here}

[StefanieSenger]

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.

[StefanieSenger]

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]

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)

[virchan]

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!

[virchan]

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.

[StefanieSenger]

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.

[StefanieSenger]

Thanks a lot for your review, @virchan!
That was pretty helpful and I have applied all your suggestions.

[virchan]

LGTM! Thanks @StefanieSenger!

@adrinjalali, would you like to have a look?

[adrinjalali]

Otherwise LGTM.

[StefanieSenger]

Thank you for reviewing, @adrinjalali! I have addressed your suggestions. Would you have another look?

[adrinjalali]

The only thing now here that I don't find natural is that the default value of the parameter would become 0 after the deprecation cycle. I think the default value should be np.nan to make sure users actually notice it's undefined, and if they want to make it something else, they can.

Having 0 as the default value for undefined seems a bit odd to me.

Not sure what @glemaitre thinks about this.

[StefanieSenger]

I'd be happy for reviews, maybe @adrinjalali would like to have a look?

[adrinjalali]

I'm a bit lost as which comments are relevant from the review history @StefanieSenger , please let me know which discussions are open.

[StefanieSenger]

I'm a bit lost as which comments are relevant from the review history @StefanieSenger , please let me know which discussions are open.

Only this one (#31172 (comment)), @adrinjalali.

Edit: I answered to your and @jeremiedbb's suggestions here. Please let me know what you think.

[StefanieSenger]

After pulling from main, the tests fail. I will investigate, but on another day. Maybe a newly added test.

[StefanieSenger]

After pulling from main, the tests fail. I will investigate, but on another day. Maybe a newly added test.

Yes, it came from #32549, where a new handling for empty inputs was added. I have removed the obsolete test case from here.

[jeremiedbb]

LGTM. Thanks !

[adrinjalali]

Minor point to improve the warning message, otherwise LGTM. Feel free to merge if CI is green after fixing the message.

[adrinjalali]

same here

[StefanieSenger]

Thanks reviewing, @virchan, @jeremiedbb and @adrinjalali. I'm happy about the result.
I've enabled auto-merge.