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

Fix generalized dice computation #7970

Merged
merged 18 commits into from
Sep 7, 2024
Merged

Fix generalized dice computation #7970

merged 18 commits into from
Sep 7, 2024

Conversation

surajpaib
Copy link
Contributor

Fixes #7966

Description

A few sentences describing the changes proposed in this pull request.

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.

@surajpaib
Fix generalized dice computation
123c778 
Signed-off-by: Suraj Pai <[email protected]>

Similar functionality to torchmetrics

Update

Lint and update sum_over_labels

Update docstring

Update docstring
@surajpaib surajpaib mentioned this pull request Aug 1, 2024
Merged
KumoLiu
KumoLiu reviewed Aug 7, 2024
View reviewed changes
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.

Thanks for the contribution, overall looks good to me.
Once you finished the PR, please let me know, I will add more people help review it, thanks.

monai/metrics/generalized_dice.py Show resolved Hide resolved
monai/metrics/generalized_dice.py Outdated Show resolved Hide resolved
@KumoLiu
Copy link
Contributor

KumoLiu commented Aug 26, 2024

Hi @surajpaib, do you plan to address the comments on the PR? I’m hoping we can get this merged into version 1.4, but we only have about one or two weeks left to add new features.

@surajpaib
Copy link
Contributor Author

Hi @KumoLiu, sorry for the gap, was away on vacation. I think I can work on this in the next few days and get it in.

@surajpaib
Copy link
Contributor Author

@KumoLiu I've incorporated the comments we discussed.

deprecated_arg_default is added to the initialization and for the reduction, I've added deprecated_arg.

The tests seem to be failing because of the version string - I assume we can set this appropriately during the release.

Let me know if you see any other changes

@surajpaib surajpaib marked this pull request as ready for review September 4, 2024 20:39
@surajpaib surajpaib requested a review from KumoLiu September 4, 2024 20:40
@surajpaib @KumoLiu
Update monai/metrics/generalized_dice.py
4ea6a2c 
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Suraj Pai <[email protected]>
@surajpaib @KumoLiu
Update monai/metrics/generalized_dice.py
2bc6dd8 
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Suraj Pai <[email protected]>
KumoLiu
KumoLiu approved these changes Sep 6, 2024
View reviewed changes
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.

Thanks for the update, LGTM.
Could you please help fix the ci issue and dco issue then I can help trigger more tests and merge this one?
https://github.com/Project-MONAI/MONAI/actions/runs/10733660015/job/29767399315?pr=7970#step:7:428

monai/metrics/generalized_dice.py Outdated Show resolved Hide resolved
tests/test_compute_generalized_dice.py Show resolved Hide resolved
@surajpaib @KumoLiu
Update monai/metrics/generalized_dice.py
72ed145 
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Suraj Pai <[email protected]>
@KumoLiu
Copy link
Contributor

KumoLiu commented Sep 6, 2024

Hi @surajpaib, you can fix the dco issue by following the method here: https://github.com/Project-MONAI/MONAI/pull/7970/checks?check_run_id=29787940944
Also please update here: https://github.com/Project-MONAI/MONAI/pull/7970/files#r1746799238
Then I will try to merge this one, thanks!

@surajpaib
Fix docs + improve tests
a762649 
_________

DCO Remediation Commit for Suraj Pai <[email protected]>

I, Suraj Pai <[email protected]>, hereby add my Signed-off-by to this commit: 2081cd2
I, Suraj Pai <[email protected]>, hereby add my Signed-off-by to this commit: 3805fea
I, Suraj Pai <[email protected]>, hereby add my Signed-off-by to this commit: fb6e660
I, Suraj Pai <[email protected]>, hereby add my Signed-off-by to this commit: 8274060

Signed-off-by: Suraj Pai <[email protected]>
@surajpaib
Merge branch 'dev' of github.com:surajpaib/MONAI into dev
bf0c5f0 
Signed-off-by: Suraj Pai <[email protected]>
@KumoLiu
Copy link
Contributor

KumoLiu commented Sep 7, 2024

/build

@KumoLiu KumoLiu merged commit d02ba11 into Project-MONAI:dev Sep 7, 2024
28 checks passed
hjmjohnson pushed a commit to hjmjohnson/MONAI that referenced this pull request Sep 8, 2024
@surajpaib @KumoLiu @hjmjohnson
Fix generalized dice computation (Project-MONAI#7970)
8f8859d 
Fixes Project-MONAI#7966 

### Description

A few sentences describing the changes proposed in this pull request.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] 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.

---------

Signed-off-by: Suraj Pai <[email protected]>
Signed-off-by: Suraj Pai <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericspod ericspod ericspod left review comments

@KumoLiu KumoLiu KumoLiu approved these changes

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

Successfully merging this pull request may close these issues.

GeneralizedDiceScore reductions do not work as expected
3 participants
@surajpaib @KumoLiu @ericspod