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

Mednext #1

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Mednext #1

merged 1 commit into from
Sep 11, 2024

Conversation

rcremese
Copy link

@rcremese rcremese commented Aug 21, 2024

Fixes # .

Description

This pull request is intended to solve some of the issues pointed by @jzielke in the MONAI pull request 8004. A lot of the updates come from the MONAI dev branch I'm using which is ahead of your mednext branch, but let me summarize the changes I made :

  • nets/mednext.py
    -- renamed MedNeXt arguments to follow @jzielke recomendations
    -- linked residual units of the encoder and decoder with one do_res flag
    -- add model variants (S, B, M, L) as described in the original paper
    -- solved code formating issues and corrected the module imports as stated in https://github.com/Project-MONAI/MONAI/blob/dev/CONTRIBUTING.md#checking-the-coding-style
  • blocks/mednext_block.py
    -- removed the author defined LayerNorm and replaced it with torch.nn.LayerNorm as only the "channel_first" case is used in block and network definition .
    -- renamed OutBlock to MedNeXtOutBlock to avoid naming confusion as the block is accessible in the API
    -- fixing module class export
  • test/test_mednext.py
    -- removed all the do_res_up_down test cases as do_res_up_down as been fused with do_res arguments
  • __init__.py
    -- made available the different versions of MedNext to the API as done by DenseNet

I should add some tests for the mednext_block.py script and evaluate the performance of the model on a benchmark used by the author (BTCV or AMOS22). I also tested all the basic unit test and they all pass so you don't have to do it ./runtests.sh --quick --unittests --disttests.

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

Hi @rcremese,

I think you'll need to rebase your work off of my branch so I can see the diff. changes.

You'll need to do something like this,

git checkout -b surajpaib-mednext mednext
git pull [email protected]:surajpaib/MONAI.git mednext
git checkout mednext
git rebase surajpaib-mednext

This will generate some merge conflicts that you'll need to fix and this will give a diff. from my branch so we can compare changes between my code and yours.

I would do it myself but I don't have push access to your branch.

Let me know if you have any troubles with it

@surajpaib
Copy link
Owner

Alternatively, if you add me as a collaborator to your fork, I could do this myself.

@rcremese
Copy link
Author

Oups, I guest I did something wrong. I sent you an invitation to be a contributor in my fork. I will also try to do some of the git trickery for my own and send you a new pull request.

@rcremese
Copy link
Author

rcremese commented Sep 2, 2024

Ok ! I did what you said and hope it will resolve the previous conflicts. Tell me if it doesn't because my git history seems grounded with commits that aren't yours.

…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
Copy link
Owner

Hi @rcremese, sorry for force-pushing on your branch but I've now updated the PR so it reflects only your changes to my code. Will do a review shortly!

@@ -53,7 +53,25 @@
from .generator import Generator
from .highresnet import HighResBlock, HighResNet
from .hovernet import Hovernet, HoVernet, HoVerNet, HoverNet
from .mednext import MedNeXt
from .mednext import (
Copy link
Owner

Choose a reason for hiding this comment

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

@rcremese I would prefer to do a factory method pattern similar to how it exists for resnets: https://github.com/Project-MONAI/MONAI/blob/59a7211070538586369afd4a01eca0a7fe2e742e/monai/networks/nets/resnet.py#L36

for consistency

Copy link
Author

@rcremese rcremese Sep 5, 2024

Choose a reason for hiding this comment

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

Well, I took exemple on densnet and SEnet which are both integrated into the core library.

Moreover, there are only 4 architecture variants in the original paper so it shouldn't polute the script that much.

Copy link
Owner

Choose a reason for hiding this comment

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

I see the inconsistency now.

Factory methods are better in this case I would say - it keeps them simpler as we are not using any functionality provided by subclassing the MedNext parent. Moreover, if we want to add custom logic like loading some pre-trained weights, these lend better too.



# Define the MedNeXt variants as reported in 10.48550/arXiv.2303.09975
class MedNeXtSmall(MedNeXt):
Copy link
Owner

Choose a reason for hiding this comment

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

Using factory methods should greatly simplify these code blocks as well!

@surajpaib
Copy link
Owner

@rcremese I'll go ahead and merge this PR - we can discuss more on my upstream

@surajpaib surajpaib merged commit bfff9a2 into surajpaib:mednext Sep 11, 2024
17 of 30 checks passed
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