Fix generalized dice computation by surajpaib · Pull Request #7970 · Project-MONAI/MONAI · GitHub
Skip to content

Fix generalized dice computation#7970

Merged
KumoLiu merged 18 commits into
Project-MONAI:devfrom
surajpaib:dev
Sep 7, 2024
Merged

Fix generalized dice computation#7970
KumoLiu merged 18 commits into
Project-MONAI:devfrom
surajpaib:dev

Conversation

@surajpaib

@surajpaib surajpaib commented Jul 31, 2024

Copy link
Copy Markdown
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 <bspai@bwh.harvard.edu>

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 1, 2024
View reviewed changes
Comment thread monai/metrics/generalized_dice.py Outdated
KumoLiu
KumoLiu reviewed Aug 7, 2024
View reviewed changes

@KumoLiu KumoLiu left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread monai/metrics/generalized_dice.py
Comment thread monai/metrics/generalized_dice.py Outdated
ericspod
ericspod reviewed Aug 7, 2024
View reviewed changes
Comment thread monai/metrics/generalized_dice.py Outdated
@KumoLiu

KumoLiu commented Aug 26, 2024

Copy link
Copy Markdown
Contributor

@surajpaib

surajpaib commented Aug 27, 2024

Copy link
Copy Markdown
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

surajpaib commented Aug 28, 2024

Copy link
Copy Markdown
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
KumoLiu
KumoLiu reviewed Sep 5, 2024
View reviewed changes
Comment thread monai/metrics/generalized_dice.py Outdated
@KumoLiu
Update monai/metrics/generalized_dice.py
4ea6a2c 
Co-authored-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Signed-off-by: Suraj Pai <b.pai@maastrichtuniversity.nl>
KumoLiu
KumoLiu reviewed Sep 5, 2024
View reviewed changes
Comment thread monai/metrics/generalized_dice.py Outdated
@KumoLiu
Update monai/metrics/generalized_dice.py
2bc6dd8 
Co-authored-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Signed-off-by: Suraj Pai <b.pai@maastrichtuniversity.nl>
KumoLiu
KumoLiu reviewed Sep 5, 2024
View reviewed changes
Comment thread monai/metrics/generalized_dice.py Outdated
KumoLiu
KumoLiu approved these changes Sep 6, 2024
View reviewed changes

@KumoLiu KumoLiu left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread monai/metrics/generalized_dice.py Outdated
Comment thread tests/test_compute_generalized_dice.py
@KumoLiu
Update monai/metrics/generalized_dice.py
72ed145 
Co-authored-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Signed-off-by: Suraj Pai <b.pai@maastrichtuniversity.nl>
@KumoLiu

KumoLiu commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

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 <bspai@bwh.harvard.edu>

I, Suraj Pai <bspai@bwh.harvard.edu>, hereby add my Signed-off-by to this commit: 2081cd2
I, Suraj Pai <bspai@bwh.harvard.edu>, hereby add my Signed-off-by to this commit: 3805fea
I, Suraj Pai <bspai@bwh.harvard.edu>, hereby add my Signed-off-by to this commit: fb6e660
I, Suraj Pai <bspai@bwh.harvard.edu>, hereby add my Signed-off-by to this commit: 8274060

Signed-off-by: Suraj Pai <bspai@bwh.harvard.edu>
@surajpaib
Merge branch 'dev' of github.com:surajpaib/MONAI into dev
bf0c5f0 
Signed-off-by: Suraj Pai <bspai@bwh.harvard.edu>
surajpaib and others added 2 commits September 6, 2024 11:48
@surajpaib
Fix flake8 error
27a0c58 
Signed-off-by: Suraj Pai <bspai@bwh.harvard.edu>
@KumoLiu
Merge branch 'dev' into dev
c704cc8
@KumoLiu

KumoLiu commented Sep 7, 2024

Copy link
Copy Markdown
Contributor

@KumoLiu KumoLiu merged commit d02ba11 into Project-MONAI:dev Sep 7, 2024
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