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 MedNext implementation #8004

Merged
merged 9 commits into from
Nov 13, 2024
Merged

Add MedNext implementation #8004

merged 9 commits into from
Nov 13, 2024

Conversation

surajpaib
Copy link
Contributor

Fixes #7786

Description

Added MedNext architectures implementation for MONAI.

Since a lot of the code is heavily sourced from the original MedNext repo, https://github.com/MIC-DKFZ/MedNeXt, I wanted to check if there is an attribution policy with regarded to borrowed source code. I've added a derivative notice bellow the monai copyright comment. Let me know if this needs to be changed.

The blocks have been taken almost as is but the network implementation has been changed largely to allow flexible blocks and follow MONAI segresnet styling.

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.

johnzielke
johnzielke suggested changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@johnzielke johnzielke 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 work! I think it's a great addition to MONAI!

monai/networks/blocks/mednext_block.py Outdated Show resolved Hide resolved
monai/networks/blocks/mednext_block.py Outdated Show resolved Hide resolved
monai/networks/nets/mednext.py Show resolved Hide resolved
monai/networks/nets/mednext.py Show resolved Hide resolved
monai/networks/nets/mednext.py Outdated Show resolved Hide resolved
monai/networks/nets/mednext.py Outdated Show resolved Hide resolved
monai/networks/nets/mednext.py Outdated Show resolved Hide resolved
monai/networks/nets/mednext.py Show resolved Hide resolved
monai/networks/blocks/mednext_block.py Outdated Show resolved Hide resolved
@rcremese
Copy link
Contributor

rcremese commented Aug 14, 2024 •
edited
Loading

Can't it be usefull to adapt the factory functions of create_mednext_v1 script inside your mednext.py script in order to implement basic configurations of the models in order to compare with the reference paper ?

rcremese added a commit to rcremese/MONAI that referenced this pull request Aug 21, 2024
@rcremese
Code formating for Blake and Flake8 checks to pass + integration of M…
b0a968b 
…edNext variants (S, B, M, L) + integration of remarks from @johnzilke (Project-MONAI#8004 (review)) for renaming class arguments - removal of self defined LayerNorm - linked residual connection for encoder and decoder

Signed-off-by: Robin CREMESE <[email protected]>
@rcremese rcremese mentioned this pull request Aug 21, 2024
Merged
7 tasks
@surajpaib
Copy link
Contributor Author

Thanks for the comments @johnzielke and thank you for the updates @rcremese. I'll update my branch to dev and look through the changes in your PR asap.

rcremese added a commit to rcremese/MONAI that referenced this pull request Sep 2, 2024
@rcremese
Code formating for Blake and Flake8 checks to pass + integration of M…
0361444 
…edNext variants (S, B, M, L) + integration of remarks from @johnzilke (Project-MONAI#8004 (review)) for renaming class arguments - removal of self defined LayerNorm - linked residual connection for encoder and decoder

Signed-off-by: Robin CREMESE <[email protected]>
rcremese and others added 2 commits September 3, 2024 15:21
@rcremese @surajpaib
Code formating for Blake and Flake8 checks to pass + integration of M…
93e782f 
…edNext variants (S, B, M, L) + integration of remarks from @johnzilke (Project-MONAI#8004 (review)) for renaming class arguments - removal of self defined LayerNorm - linked residual connection for encoder and decoder

Signed-off-by: Robin CREMESE <[email protected]>
@surajpaib
Update mednext implementations
e146aaf 
Signed-off-by: Suraj Pai <[email protected]>
@surajpaib surajpaib marked this pull request as ready for review September 27, 2024 23:34
@surajpaib
Fix mypy errors
17d0579 
Signed-off-by: Suraj Pai <[email protected]>
@surajpaib
Copy link
Contributor Author

@KumoLiu This implementation should be ready now. Please let me know if you have any comments

KumoLiu
KumoLiu reviewed Sep 30, 2024
View reviewed changes
monai/networks/blocks/mednext_block.py Show resolved Hide resolved
monai/networks/blocks/mednext_block.py Outdated Show resolved Hide resolved
monai/networks/blocks/mednext_block.py Show resolved Hide resolved
monai/networks/blocks/mednext_block.py Show resolved Hide resolved
@surajpaib
Add docstrings
efe93b2 
Signed-off-by: Suraj Pai <[email protected]>
@surajpaib
Copy link
Contributor Author

@KumoLiu Added.

Do you think there would be interest to add this as a candidate for Auto3DSeg? I refer to this paper for its performance benchmarking: https://arxiv.org/abs/2404.09556

@surajpaib surajpaib requested a review from KumoLiu October 3, 2024 18:02
@KumoLiu KumoLiu requested review from ericspod and Nic-Ma October 4, 2024 14:31
@KumoLiu
Copy link
Contributor

KumoLiu commented Oct 4, 2024

Do you think there would be interest to add this as a candidate for Auto3DSeg? I refer to this paper for its performance benchmarking: https://arxiv.org/abs/2404.09556

Thank you for bringing this up, it's an interesting suggestion. I believe it could be worthwhile to consider this as a potential candidate for Auto3DSeg. However, before moving forward, I would appreciate hearing others' thoughts and insights on whether this aligns with the current goals and roadmap for Auto3DSeg. cc @mingxin-zheng @dongyang0122 @Nic-Ma @myron

@mrcolo
Copy link

mrcolo commented Oct 10, 2024

Great work @surajpaib ! any plans on when we're gonna be able to get this into main?

@surajpaib
Copy link
Contributor Author

Hi @KumoLiu, any update on merging this?

We can maybe have a separate issue to discuss Auto3DSeg integration then

@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 11, 2024

/build

@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 11, 2024

Hi @KumoLiu, any update on merging this?

Hi @surajpaib, I noticed there are still several open comments. Do you plan to address these, or would it be easier just to respond if you don't intend to make updates? Thanks!

@surajpaib
Copy link
Contributor Author

@KumoLiu I've closed the comments as most of them have been addressed over modifications made by @rcremese. Also addressed a few that weren't.

@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 12, 2024

/build

@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 13, 2024

/build

@KumoLiu KumoLiu merged commit 941e739 into Project-MONAI:dev Nov 13, 2024
28 checks passed
@surajpaib surajpaib mentioned this pull request Nov 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcremese rcremese rcremese left review comments

@johnzielke johnzielke johnzielke requested changes

@zephyrie zephyrie zephyrie left review comments

@aylward aylward aylward left review comments

@KumoLiu KumoLiu KumoLiu approved these changes

@ericspod ericspod Awaiting requested review from ericspod

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

Add MedNeXt model architectures within MONAI
8 participants
@surajpaib @rcremese @KumoLiu @mrcolo @aylward @zephyrie @ericspod @johnzielke