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.

Sign up for GitHub

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

Jump to bottom

Add alpha parameter to DiceFocalLoss #7841

Merged
merged 11 commits into from
Jun 29, 2024
Merged

Add alpha parameter to DiceFocalLoss #7841

merged 11 commits into from
Jun 29, 2024

Conversation

kephale
Copy link
Contributor

@kephale kephale commented Jun 12, 2024

Fixes #7682.

Description

This PR introduces the alpha parameter from FocalLoss into DiceFocalLoss.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.
  • 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.

@kephale
Add alpha parameter to DiceFocalLoss
8693ada
@kephale
Add a test for alpha usage in DiceFocalLoss
06a2509
@kephale
Apply black
7eec525
@kephale
DCO Remediation Commit for Kyle Harrington <czi@kyleharrington.com>
ec45af2 
I, Kyle Harrington <czi@kyleharrington.com>, hereby add my Signed-off-by to this commit: 8693ada
I, Kyle Harrington <czi@kyleharrington.com>, hereby add my Signed-off-by to this commit: 06a2509
I, Kyle Harrington <czi@kyleharrington.com>, hereby add my Signed-off-by to this commit: 7eec525

Signed-off-by: Kyle Harrington <czi@kyleharrington.com>
ericspod
ericspod reviewed Jun 18, 2024
View reviewed changes
monai/losses/dice.py Outdated Show resolved Hide resolved
@ericspod
Copy link
Member

Hi @kephale thanks for the contribution! I've made a few inline comments but in general it looks good. Please address these and I can come back to it for approval.

@ericspod ericspod requested review from Nic-Ma and KumoLiu June 18, 2024 12:05
@kephale
Copy link
Contributor Author

kephale commented Jun 18, 2024

Okey doke, the requested changes have been addressed. I'm not sure what is going on with the min-dep tests because that part of the code hasn't been modified.

@ericspod
Copy link
Member

The test fails are related to Numpy 2.0 being released a few days ago and we aren't compatible yet. I have a fix I need to push then these will work.

@ericspod
Copy link
Member

I have pushed a temporary fix now so I'm rerunning your tests after merging dev into this branch.

@ericspod
Copy link
Member

There is a DCO fail that you do need to fix, @kephale. The instructions are there if you click on the check.

@kephale
DCO Remediation Commit for Kyle Harrington <czi@kyleharrington.com>
a590b49 
I, Kyle Harrington <czi@kyleharrington.com>, hereby add my Signed-off-by to this commit: 10c82c6
I, Kyle Harrington <czi@kyleharrington.com>, hereby add my Signed-off-by to this commit: 303a257
I, Kyle Harrington <czi@kyleharrington.com>, hereby add my Signed-off-by to this commit: 0f47476

Signed-off-by: Kyle Harrington <czi@kyleharrington.com>
@kephale
Copy link
Contributor Author

kephale commented Jun 28, 2024

The DCO should be addressed.

@KumoLiu
Copy link
Contributor

KumoLiu commented Jun 28, 2024

/build

@KumoLiu
Copy link
Contributor

KumoLiu commented Jun 28, 2024

Hi @ericspod, do you have any other comments on this one?

ericspod
ericspod approved these changes Jun 29, 2024
View reviewed changes
Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

I'm good here!

@ericspod ericspod merged commit 2f62b81 into Project-MONAI:dev Jun 29, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericspod ericspod ericspod approved these changes

@KumoLiu KumoLiu KumoLiu approved these changes

@Nic-Ma Nic-Ma Awaiting requested review from Nic-Ma

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing alpha parameter of Focal loss in DiceFocalLoss
3 participants
@kephale @ericspod @KumoLiu