-
-
Notifications
You must be signed in to change notification settings - Fork 655
add cohen kappa #1690
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
add cohen kappa #1690
Conversation
@vfdev-5. Putting try/except in the |
Hey @KickItLikeShika , what's the issue with try/except in the As for other updates, please, take a look at a similar PR here and the comments about the documentation : #1682 As discussed in your previous closed PR, please use a single PR to bring all the updates according to the review. Otherwise, we wont be able to consider them. |
@vfdev-5. I have fixed it. Is there anything else to edit? |
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.
@KickItLikeShika few more comments to address, please.
ad82aee
to
3037b3f
Compare
@vfdev-5. I have updated everything as you said and added the test. But please look at these CI errors, it works fine locally! and look at the values, it so close! That's weird! |
@KickItLikeShika sounds great for the update ! Yes, it can happen that values could differ with a tiny eps. When you say that it works locally, how do you run the tests ? |
|
If you'd like to replicate distributed tests on CPU, you have to replicate the command from
and make sure to install |
Okay thank you! Is there anything else to work on in this pull request? |
And to fix precision issue, you can use |
3c94071
to
20419c5
Compare
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.
@KickItLikeShika thanks for a thourough testing !
I have a remark about duplicating test functions and usage of parametrize.
ck._check_shape((torch.randint(0, 2, size=(10, 1)).long(), torch.randint(0, 2, size=(10, 5, 2)).long())) | ||
|
||
|
||
def test_cohen_kappa_non_weighted(): |
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.
Let's try to use pytest.mark.parametrize
here. For example, take a look here : https://github.com/pytorch/ignite/pull/1683/files#diff-be88f17b02401b784cf4081d7e84bc7470d9beac1ac42294b8a0b1fc731900b3R20
Such that we can refactor all those 3 almost idenitcal functions. Probably, a similar thing can be done for test_integration_cohen_kappa_non_weighted_with_output_transform and its 2 triplets...
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 understood how it works. I can use test.mark.parametrize
to replace all of these function with just 2 or 3. Should i do that?
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 understood how it works. I can use test.mark.parametrize to replace all of these function with just 2 or 3. Should i do that?
The idea is to make test code better with minimal code duplication:
3 test functions -> 1 test function with parametrize
test_cohen_kappa_*
->test_cohen_kappa
test_integration_cohen_kappa_*_with_output_transform
->test_integration_cohen_kappa_with_output_transform
Sorry, I do not quite get your question...
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.
@vfdev-5, Updated all the tests, and added new tests.
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.
@vfdev-5, It breaks the CI. The issue is the code formatting, black
and pycodestyle
don't agree on whitespaces before the columns. Any idea how to fix this?
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.
Aftert running black test_cohen_kappa.py
the columns in lines 98, 107, 108 get reformatted and whitespaces added before the columns :
. But then pycodestyle test_cohen_kappy.py
shows that i should remove the whitespaces before the columns np_y[size // 2 :] = 1
. And that's how the unit tests in the CI fail
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.
@vfdev-5, should i change the logic in update_fn
in test_cohen_kappa_all_weights_with_output_transform
for avoid these CI errors?
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.
@KickItLikeShika I do not quite understand your problem with formatting here. I updated your branch to the master to restart the CI and see where it is failing.
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.
Looks like it is OK.
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.
Well! i will commit the latest updates now with and delete the tests you told my about
@KickItLikeShika as a suggestion, try to enable Github Actions on your fork such that our "Format python code / code-style" could commit fixed files to your fork and avoid manual fixing. |
07a316b
to
d2fb323
Compare
b3c77a9
to
ff240fa
Compare
531169d
to
a1491b0
Compare
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 for the PR @KickItLikeShika ! Looks good now 👍
Actually, I was thinking that XLA test failure is unrelated but it is related:
as it does not fail on other PR, nor on master. Let me see in details later what happens... |
Do you have any idea what might be the problem here? |
@vfdev-5, Do you think i have to edit anything before i change the rest of the metrics to this similar implementation? |
Well, the test with XLA should be fixed. As far as I can see from 61e5041#diff-54ddf6f7687e40789e7b5180b452037c40cb021140ae45ed5297d5698a6a8dbeR164 we disabled accumulation on XLA devices (unfortunately without any explanation). We have to invetigate this more in details. |
@vfdev-5 then i will start improving the other metrics the same way and add the tests |
@KickItLikeShika finally get it done 🎉 Congrats :) |
@vfdev-5, Thank you!! |
Fixes #1673
Added Cohen's Kappa in
ignite.contrib.metrics
. Implemented 'CohenKappa' incohen_kappa.py
file. And added the tests intest_cohen_kappa.py
.Check list: