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

[Fix] Added ignore_index and one hot encoding for dice loss #3237

Merged
merged 6 commits into from
Aug 9, 2023

Conversation

yeedrag
Copy link
Contributor

@yeedrag yeedrag commented Jul 28, 2023

Added ignore_index param to forward(),
also implemented one hot encoding to ensure the dims of target matches pred.

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

Please describe the motivation of this PR and the goal you want to achieve through this PR.

Attempted to solve the problems mentioned by #3172

Modification

Please briefly describe what modification is made in this PR.

Added ignore_index into forward function (although the dice loss itself does not actually take account for it for some reason).
Added _expand_onehot_labels_dice, which takes the target with shape [N, H, W] into [N, num_classes, H, W].

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.

This is my first time contributing to open-source code, so I might have made some stupid mistakes. Please don't hesitate to point it out.

Added ignore_index param to forward(),
also implemented one hot encoding to ensure the dims of target match pred.
@CLAassistant
Copy link

CLAassistant commented Jul 28, 2023

CLA assistant check
All committers have signed the CLA.

@yeedrag yeedrag changed the title Added ignore_index and one hot encoding Added ignore_index and one hot encoding for dice loss Jul 29, 2023
@yeedrag yeedrag changed the title Added ignore_index and one hot encoding for dice loss [Fix] Added ignore_index and one hot encoding for dice loss Jul 30, 2023
mmseg/models/losses/dice_loss.py Outdated Show resolved Hide resolved
mmseg/models/losses/dice_loss.py Outdated Show resolved Hide resolved
Changed as the syntax a | b (equal to typing.Union[a, b]) only works above python 3.9.
@yeedrag
Copy link
Contributor Author

yeedrag commented Aug 1, 2023

Hmm, I'm not entirely sure why the pr_stage_test failed after adding the type hints...
edit: I think its a problem with Circleci, because multiple other prs are facing the same fail.

@xiexinch
Copy link
Collaborator

xiexinch commented Aug 2, 2023

Hmm, I'm not entirely sure why the pr_stage_test failed after adding the type hints... edit: I think its a problem with Circleci, because multiple other prs are facing the same fail.

It's caused by the update of MMEngine...
We're working on fixing it.

@OpenMMLab-Assistant-004
Copy link

Hi @yeedrag,

We'd like to express our appreciation for your valuable contributions to the mmsegmentation. Your efforts have significantly aided in enhancing the project's quality.
It is our pleasure to invite you to join our community thorugh Discord_Special Interest Group (SIG) channel. This is a great place to share your experiences, discuss ideas, and connect with other like-minded people. To become a part of the SIG channel, send a message to the moderator, OpenMMLab, briefly introduce yourself and mention your open-source contributions in the #introductions channel. Our team will gladly facilitate your entry. We eagerly await your presence. Please follow this link to join us: ​https://discord.gg/UjgXkPWNqA.

If you're on WeChat, we'd also love for you to join our community there. Just add our assistant using the WeChat ID: openmmlabwx. When sending the friend request, remember to include the remark "mmsig + Github ID".

Thanks again for your awesome contribution, and we're excited to have you as part of our community!

@yeedrag
Copy link
Contributor Author

yeedrag commented Aug 2, 2023

Also, with my current pr, I haven't implemented actually using ignore_index to calculate the loss. I think I can implement it, but should I just commit the implementation in this pr or should I make a new one? Thanks.

@xiexinch
Copy link
Collaborator

xiexinch commented Aug 2, 2023

Also, with my current pr, I haven't implemented actually using ignore_index to calculate the loss. I think I can implement it, but should I just commit the implementation in this pr or should I make a new one? Thanks.

Might commit to this pr.

@xiexinch
Copy link
Collaborator

xiexinch commented Aug 2, 2023

Could you help to update some code?

if self.activate:
    if self.use_sigmoid:
        pred = pred.sigmoid()
    else:
        pred = pred.softmax(dim=1)

The current implementation only supports sigmoid, we might refer to the implementation at MONAI(https://github.com/Project-MONAI/MONAI/blob/dev/monai/losses/dice.py#L140).

@yeedrag
Copy link
Contributor Author

yeedrag commented Aug 2, 2023

Could you help to update some code?

if self.activate:
    if self.use_sigmoid:
        pred = pred.sigmoid()
    else:
        pred = pred.softmax(dim=1)

The current implementation only supports sigmoid, we might refer to the implementation at MONAI(https://github.com/Project-MONAI/MONAI/blob/dev/monai/losses/dice.py#L140).

So I add softmax and other_act?

@xiexinch
Copy link
Collaborator

xiexinch commented Aug 2, 2023

Could you help to update some code?

if self.activate:
    if self.use_sigmoid:
        pred = pred.sigmoid()
    else:
        pred = pred.softmax(dim=1)

The current implementation only supports sigmoid, we might refer to the implementation at MONAI(https://github.com/Project-MONAI/MONAI/blob/dev/monai/losses/dice.py#L140).

So I add softmax and other_act?

Currently, we only need to add softmax, if you have a better solution, you can support adding other activation methods.

@yeedrag
Copy link
Contributor Author

yeedrag commented Aug 2, 2023

Currently, we only need to add softmax, if you have a better solution, you can support adding other activation methods.

Done!

if self.activate:
    if self.use_sigmoid:
        pred = pred.sigmoid()
    elif pred.shape[1] != 1:
        # softmax does not work when there is only 1 class
        pred = pred.softmax(dim=1)

I'm not sure whether to use an assert or a warning when there's only 1 class so I currently just used an elif.

@xiexinch xiexinch merged commit 817c18b into open-mmlab:dev-1.x Aug 9, 2023
2 checks passed
angiecao pushed a commit to angiecao/mmsegmentation that referenced this pull request Aug 31, 2023
…ab#3237)

Added ignore_index param to forward(),
also implemented one hot encoding to ensure the dims of target matches
pred.

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

Please describe the motivation of this PR and the goal you want to
achieve through this PR.

Attempted to solve the problems mentioned by open-mmlab#3172 

## Modification

Please briefly describe what modification is made in this PR.

Added ignore_index into forward function (although the dice loss itself
does not actually take account for it for some reason).
Added _expand_onehot_labels_dice, which takes the target with shape [N,
H, W] into [N, num_classes, H, W].

## 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.

This is my first time contributing to open-source code, so I might have
made some stupid mistakes. Please don't hesitate to point it out.
emily-lin pushed a commit to emily-lin/mmsegmentation that referenced this pull request Nov 18, 2023
…ab#3237)

Added ignore_index param to forward(),
also implemented one hot encoding to ensure the dims of target matches
pred.

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

Please describe the motivation of this PR and the goal you want to
achieve through this PR.

Attempted to solve the problems mentioned by open-mmlab#3172 

## Modification

Please briefly describe what modification is made in this PR.

Added ignore_index into forward function (although the dice loss itself
does not actually take account for it for some reason).
Added _expand_onehot_labels_dice, which takes the target with shape [N,
H, W] into [N, num_classes, H, W].

## 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.

This is my first time contributing to open-source code, so I might have
made some stupid mistakes. Please don't hesitate to point it out.
nahidnazifi87 pushed a commit to nahidnazifi87/mmsegmentation_playground that referenced this pull request Apr 5, 2024
…ab#3237)

Added ignore_index param to forward(),
also implemented one hot encoding to ensure the dims of target matches
pred.

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

Please describe the motivation of this PR and the goal you want to
achieve through this PR.

Attempted to solve the problems mentioned by open-mmlab#3172 

## Modification

Please briefly describe what modification is made in this PR.

Added ignore_index into forward function (although the dice loss itself
does not actually take account for it for some reason).
Added _expand_onehot_labels_dice, which takes the target with shape [N,
H, W] into [N, num_classes, H, W].

## 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.

This is my first time contributing to open-source code, so I might have
made some stupid mistakes. Please don't hesitate to point it out.
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.

4 participants