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

[Feature] Add support for the focal Tversky loss #2783

Closed
wants to merge 9 commits into from
Closed

[Feature] Add support for the focal Tversky loss #2783

wants to merge 9 commits into from

Conversation

zifuwanggg
Copy link

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

The focal Tversky loss was proposed in https://arxiv.org/abs/1810.07842. It has nearly 600 citations and has been shown to be extremely useful for highly imbalanced (medical) datasets. To add support for the focal Tversky loss, only few lines of changes are needed for the Tversky loss.

Modification

Add gamma as (optional) argument in the constructor of TverskyLoss. This parameter is then passed to tversky_loss to compute the focal Tversky loss.

BC-breaking (Optional)

Does the modification introduce changes that break the backward-compatibility of the downstream repos?
If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.

Use cases (Optional)

If this PR introduces a new feature, it is better to list some use cases here, and update the documentation.

Checklist

  1. Pre-commit or other linting tools are used to fix the potential lint issues.
  2. The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  3. If the modification has potential influence on downstream projects, this PR should be tested with downstream projects, like MMDet or MMDet3D.
  4. The documentation has been modified accordingly, like docstring or example tutorials.

@CLAassistant
Copy link

CLAassistant commented Mar 22, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ zifuwanggg
❌ Zifu Wang


Zifu Wang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@zifuwanggg
Copy link
Author

It seems that I have signed CLA but still get complains.

Screen Shot 2023-03-22 at 22 26 13

Screen Shot 2023-03-22 at 22 26 26

@xiexinch xiexinch changed the title Add support for the focal Tversky loss [Feature] Add support for the focal Tversky loss Mar 23, 2023
@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.02 ⚠️

Comparison is base (d6079bc) 88.36% compared to head (af32881) 88.34%.

❗ Current head af32881 differs from pull request most recent head b618e06. Consider uploading reports for the commit b618e06 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2783      +/-   ##
==========================================
- Coverage   88.36%   88.34%   -0.02%     
==========================================
  Files         149      149              
  Lines        9109     9112       +3     
  Branches     1523     1524       +1     
==========================================
+ Hits         8049     8050       +1     
- Misses        810      811       +1     
- Partials      250      251       +1     
Flag Coverage Δ
unittests 88.34% <33.33%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmseg/models/losses/tversky_loss.py 91.22% <33.33%> (-3.22%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xiexinch
Copy link
Collaborator

Hi @zifuwanggg,
Thanks for your contribution!
You can check if the email used in the local commit is the same as the email in the GitHub profile, if not, please add the email used in the commit to the GitHub profile.

@@ -75,6 +78,9 @@ class TverskyLoss(nn.Module):
beta (float, in [0, 1]):
The coefficient of false negatives. Default: 0.7.
Note: alpha + beta = 1.
gamma (float, in [1, 3]): The focal term. When `gamma` > 1, the loss
Copy link
Contributor

@MengzhangLI MengzhangLI Mar 23, 2023

Choose a reason for hiding this comment

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

We may add assertion below to ensure 1.0 < gamma < 3.0, so does unit test.

Copy link
Author

@zifuwanggg zifuwanggg Mar 23, 2023

Choose a reason for hiding this comment

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

The [1, 3] range was mentioned in the paper, although they do not mention why gamma should be in this range. Theoretically, gamma can be any real number. Practically, a small gamma (e.g. < 2) works better. mmseg.models.losses.focal_loss does not specify the range of gamma, so perhaps we should remove the requirement for gamma in [1, 3] and let the user to decide?

Copy link
Author

Choose a reason for hiding this comment

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

Beside, I also think alpha and beta in the original TverskyLoss class does not need to satisfy assert alpha + beta == 1.0. Floating point equality is dangerous and according the definition of the Tversky index, the only requirement for alpha and beta is that they are non-negative numbers.

Comment on lines 35 to 36
if gamma > 1:
tversky_loss **= (1 / gamma)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might add a unit test.

@zifuwanggg
Copy link
Author

zifuwanggg commented Mar 23, 2023

Hi, @xiexinch

Thanks for the comment. The email I used for the very first commits was indeed by default username@localhost.local. Even if I add that email to my GitHub account, I cannot verify that email address since it cannot receive the verification email from GitHub. Modify the email address for previous commits are strongly discouraged. Is there a workaround to let me sign the CLA, or I might just close this one and open a new pull request?

@xiexinch
Copy link
Collaborator

Hi, @xiexinch

Thanks for the comment. The email I used for the very first commits was indeed by default username@localhost.local. Even if I add that email to my GitHub account, I cannot verify that email address since it cannot receive the verification email from GitHub. Modify the email address for previous commits are strongly discouraged. Is there a workaround to let me sign the CLA, or I might just close this one and open a new pull request?

Hi @zifuwanggg,
In that case, I think it's better to close this pull request and create a new one :)

@zifuwanggg zifuwanggg closed this Mar 24, 2023
@zifuwanggg zifuwanggg deleted the the-focal-tversky-loss branch March 24, 2023 09:30
@OpenMMLab-Assistant-007

Hi
@zifuwanggg
We are grateful for your efforts in helping improve this open-source project during your personal time.
Welcome to join OpenMMLab Special Interest Group (SIG) private channel on Discord, where you can share your experiences, ideas, and build connections with like-minded peers. To join the SIG channel, simply message moderator— OpenMMLab on Discord or briefly share your open-source contributions in the #introductions channel and we will assist you. Look forward to seeing you there! Join us :https://discord.gg/UjgXkPWNqA
If you have a WeChat account,welcome to join our community on WeChat. You can add our assistant :openmmlabwx. Please add "mmsig + Github ID" as a remark when adding friends:)
Thank you again for your contribution❤

xiexinch pushed a commit that referenced this pull request May 12, 2023
Thanks for your contribution and we appreciate it a lot. The following
instructions would make your pull request more healthy and more easily
get feedback. If you do not understand some items, don't worry, just
make the pull request and seek help from maintainers.

## Motivation

The focal Tversky loss was proposed in https://arxiv.org/abs/1810.07842.
It has nearly 600 citations and has been shown to be extremely useful
for highly imbalanced (medical) datasets. To add support for the focal
Tversky loss, only few lines of changes are needed for the Tversky loss.

## Modification

Add `gamma` as (optional) argument in the constructor of `TverskyLoss`.
This parameter is then passed to `tversky_loss` to compute the focal
Tversky loss.

## BC-breaking (Optional)

Does the modification introduce changes that break the
backward-compatibility of the downstream repos?
If so, please describe how it breaks the compatibility and how the
downstream projects should modify their code to keep compatibility with
this PR.

## Use cases (Optional)

If this PR introduces a new feature, it is better to list some use cases
here, and update the documentation.

## Checklist

1. Pre-commit or other linting tools are used to fix the potential lint
issues.
2. The modification is covered by complete unit tests. If not, please
add more unit test to ensure the correctness.
3. If the modification has potential influence on downstream projects,
this PR should be tested with downstream projects, like MMDet or
MMDet3D.
4. The documentation has been modified accordingly, like docstring or
example tutorials.

Reopening of previous
[PR](#2783).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants