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

harmonization and clarification of dice losses variants docs and associated tests #7587

Merged
merged 6 commits into from
Apr 5, 2024

Conversation

Lucas-rbnt
Copy link
Contributor

Description

This PR aims to clarify and harmonise the code for the DiceLoss variants in the monai/losses/dice.py file. With the to_onehot_y softmax and sigmoid arguments, I didn't necessarily understand the ValueError that occurred when I passed a target of size NH[WD]. I had a bit of trouble reading the documentation and understanding it. I thought that they had to be the same shape as they are displayed, unlike the number of dimensions in the input, so I added that.
Besides, in the documentation is written:

"""
raises:
      ValueError: When number of channels for target is neither 1 nor the same as input.

"""

Trying to reproduce this, we give an input with a number of channels $N$ and target a number of channels of $M$, with $M \neq N$ and $M > 1$.

loss = DiceCELoss()
input = torch.rand(1, 4, 3, 3)
target = torch.randn(1, 2, 3, 3)
loss(input, target)
>: AssertionError: ground truth has different shape (torch.Size([1, 2, 3, 3])) from input (torch.Size([1, 4, 3, 3]))

This error in the Dice is an AssertionError and not a ValueError as expected and the explanation can be confusing and doesn't give a clear idea of the error here. The classes concerned and harmonised are DiceFocalLoss, DiceCELoss and GeneralizedDiceFocalLoss with the addition of tests that behave correctly and handle this harmonisation.

Also, feel free to modify or make suggestions regarding the changes made in the docstring to make them more understandable (in my opinion, but other readers and users will probably have a different view).

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

…ciated tests

Signed-off-by: Lucas Robinet <robinet.lucas@iuct-oncopole.fr>
Signed-off-by: Lucas Robinet <robinet.lucas@iuct-oncopole.fr>
Copy link
Contributor

@KumoLiu KumoLiu left a comment

Choose a reason for hiding this comment

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

Thank you for this pull request, overall, the updates are looking good to me. However, I do have two minor comments inline for your consideration.

monai/losses/dice.py Outdated Show resolved Hide resolved
monai/losses/dice.py Outdated Show resolved Hide resolved
Lucas-rbnt and others added 4 commits April 5, 2024 09:09
Co-authored-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Signed-off-by: Lucas Robinet <luca.robinet@gmail.com>
Signed-off-by: Lucas Robinet <robinet.lucas@iuct-oncopole.fr>
@KumoLiu
Copy link
Contributor

KumoLiu commented Apr 5, 2024

/build

@KumoLiu KumoLiu merged commit 625967c into Project-MONAI:dev Apr 5, 2024
28 checks passed
@Lucas-rbnt Lucas-rbnt deleted the diceceloss-doc branch October 15, 2024 13:33
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.

2 participants