Skip to content
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 Soft Weighted Kappa Loss #762

Merged
merged 37 commits into from
Apr 13, 2020
Merged

Conversation

wenmin-wu
Copy link
Contributor

Quadratic Weighted Kappa metric is widely used in both real-world and Kaggle competitions. Some people optimize this metric by using cross-entropy loss or MSE followed by thresholds tuning.

However, it's better to find a differentiable(soft) kappa loss function which can optimize the Quadratic Weighted Kappa metric directly. I implemented a soft version which proposed in this paper: https://www.sciencedirect.com/science/article/abs/pii/S0167865517301666

I have used it in the Data Science Bowl 2019 Kaggle Competition, the notebook talking about this is here: https://www.kaggle.com/wuwenmin/dnn-and-effective-soft-quadratic-kappa-loss

And also this medium https://medium.com/@wenmin_wu/directly-optimize-quadratic-weighted-kappa-with-tf2-0-482cf0318d74 posted by me about how to implement the weighted kappa with TF2.0.

I think it's better to put this in tensorflow addons, so more people can directly call it from addon packages.

@wenmin-wu
Copy link
Contributor Author

Hi @seanpmorgan, I have reformatted the codes, can you help me re-run the tests again?

@wenmin-wu
Copy link
Contributor Author

Hi @seanpmorgan , sorry for disturbing you again. Two lines of the codes exceeded the maximum line length(79), however, they were within comments and hadn't been automatically formatted. So I shorten them manually. Could you please trigger testing run again?

Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I do suppose this shares lots of similar codes with https://github.com/tensorflow/addons/blob/master/tensorflow_addons/metrics/cohens_kappa.py
Could we somehow create some utility functions to reduce the duplicated codes?

Also, please open an issue before filing a PR next time so that the community could track/discuss in advance! Thank you!

@WindQAQ
Copy link
Member

WindQAQ commented Dec 19, 2019

Hi @seanpmorgan , sorry for disturbing you again. Two lines of the codes exceeded the maximum line length(79), however, they were within comments and hadn't been automatically formatted. So I shorten them manually. Could you please trigger testing run again?

Definitely okay! Also do not forget to update README.md under the directory.

EDIT:
ohoh, I found that you do not put the testing file into bazel BUILD. Could you update BUILD like other losses do?

@wenmin-wu
Copy link
Contributor Author

Thanks for the contribution! I do suppose this shares lots of similar codes with https://github.com/tensorflow/addons/blob/master/tensorflow_addons/metrics/cohens_kappa.py
Could we somehow create some utility functions to reduce the duplicated codes?

Also, please open an issue before filing a PR next time so that the community could track/discuss in advance! Thank you!

No, loss calculation is quite different from the metric calculation. Since loss is based on the probabilities while metric use argmax then everything is based on the discrete class labels.

@WindQAQ
Copy link
Member

WindQAQ commented Dec 20, 2019

Got it. Could you update the following files?

  • __init__.py: import your module
  • README.md: drop some module info, and your contact info if you want
  • BUILD: use bazel to run test (simply follow what other losses do)

Thanks.

@wenmin-wu
Copy link
Contributor Author

wenmin-wu commented Dec 21, 2019

@WindQAQ

Got it. Could you update the following files?

  • init.py: import your module
  • README.md: drop some module info, and your contact info if you want
  • BUILD: use bazel to run test (simply follow what other losses do)

Thanks.

Updated:)

WindQAQ
WindQAQ previously requested changes Jan 8, 2020
Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! Remember to put kappa_loss.py here so that bazel could build this module.

tensorflow_addons/losses/kappa_loss.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss_test.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss_test.py Outdated Show resolved Hide resolved
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@wenmin-wu
Copy link
Contributor Author

wenmin-wu commented Apr 10, 2020

Hi @gabrieldemarmiesse , I refined the code according to your review, and all the tests passed. Thanks for your help.

@gabrieldemarmiesse
Copy link
Member

Great! I'll take a look when I have some free time

@wenmin-wu
Copy link
Contributor Author

@gabrieldemarmiesse thanks

Copy link
Member

@gabrieldemarmiesse gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I took a quick look, it looks great, just a few changes I think we need to do.

tensorflow_addons/losses/kappa_loss_test.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss_test.py Outdated Show resolved Hide resolved
@wenmin-wu
Copy link
Contributor Author

Hi @gabrieldemarmiesse, I have made changes according to your code review, and all tests passed. Could you please have a look when you have time? thanks.

Copy link
Member

@gabrieldemarmiesse gabrieldemarmiesse 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 the changes. This pull request is great and could technically be merged now, but I'm very picky with the tests and readability since we're a small team of maintainers and we have to make sure this will give us very little maintenance in the future. Here are some ideas of improvements to make this PR perfect, if you need some clarifications/help, don't hesitate to ask

tensorflow_addons/losses/tests/kappa_loss_test.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/kappa_loss.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/tests/kappa_loss_test.py Outdated Show resolved Hide resolved
tensorflow_addons/losses/tests/kappa_loss_test.py Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
@wenmin-wu
Copy link
Contributor Author

wenmin-wu commented Apr 13, 2020

Hi @gabrieldemarmiesse , I changed the codes according to your review. All tests passed except one rnn tests. I don't know the reason, I tested them locally, it passed. Could you please have a look?

#23 241.0             "projection_initializer": "glorot_uniform",
#23 241.0         }
#23 241.0         config = cell.get_config()
#23 241.0 >       assert config == expected_config
#23 241.0 E       AssertionError: assert {'bias_initia..._cell_2', ...} == {'bias_initia..._cell_3', ...}
#23 241.0 E         Omitting 9 identical items, use -vv to show
#23 241.0 E         Differing items:
#23 241.0 E         {'name': 'nas_cell_2'} != {'name': 'nas_cell_3'}
#23 241.0 E         Full diff:
#23 241.0 E           {
#23 241.0 E            'bias_initializer': 'zeros',
#23 241.0 E            'dtype': 'float32',...
#23 241.0 E         
#23 241.0 E         ...Full output truncated (13 lines hidden), use '-vv' to show
#23 241.0 
#23 241.0 tensorflow_addons/rnn/tests/cell_test.py:241: AssertionError

https://github.com/tensorflow/addons/pull/762/checks?check_run_id=582034839

@gabrieldemarmiesse
Copy link
Member

Thank you for the changes. Yeah, the error is unrelated to your pull request, I don't know why this happens. Don't worry about it, I'll review today. We don't need to fix the unrelated error to merge.

Copy link
Member

@gabrieldemarmiesse gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks a lot for the pull request, we'll merge once the tests are passing.

@gabrieldemarmiesse
Copy link
Member

Thanks again for the pull request!

@gabrieldemarmiesse gabrieldemarmiesse merged commit 6810fb3 into tensorflow:master Apr 13, 2020
@wenmin-wu
Copy link
Contributor Author

Hi @gabrieldemarmiesse , thanks a lot for your help. Learnt a lot of code conventions from you.

jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020
* add weighted kappa loss

* add unit tests

* change some docs

* change python files format

* shorten some lines

* rename and update README and BUILD

* resolve conversations

* resolve converstions

* remove escape

* reformat tensorflow_addons/losses/kappa_loss* with black

* reformat code

* reformat code

* reformat code with black

* Update tensorflow_addons/losses/kappa_loss.py

Co-Authored-By: Gabriel de Marmiesse <[email protected]>

* [KappaLoss] change according to review

* Update tensorflow_addons/losses/kappa_loss.py

Co-Authored-By: Gabriel de Marmiesse <[email protected]>

* Update tensorflow_addons/losses/kappa_loss.py

Co-Authored-By: Gabriel de Marmiesse <[email protected]>

* [KappaLoss] change accroding to code review

* [KappaLoss] change code format

* [SoftKappaLoss] mv kappa_loss_test.py to losses/tests

* Update .github/CODEOWNERS

Co-Authored-By: Gabriel de Marmiesse <[email protected]>

* [SoftKappaLoss] refine codes according to code review

* [SoftKappaLoss] reformat codes

* [SoftKappaLoss] fix np_deep not defined

* [SoftKappaLoss] fix tests problem

* [SoftKappaLoss] unnecessary change to tigger CI

* Default value for the seed is not needed.

Co-authored-by: gabrieldemarmiesse <[email protected]>
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.

6 participants