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

Restormer Implementation #8312

Merged
merged 75 commits into from
Mar 27, 2025
Merged

Restormer Implementation #8312

merged 75 commits into from
Mar 27, 2025

Conversation

phisanti
Copy link
Contributor

Fixes # .

Description

This PR implements the Restormer architecture for high-resolution image restoration in MONAI following the discussion in issue #8261. The implementation supports both 2D and 3D images using MONAI's convolution as the base. Key additions include:

  • Downsample class for efficient downsampling operations
  • pixel_unshuffle operation complementing existing pixel_shuffle
  • Channel Attention Block (CABlock) with FeedForward layer
  • Multi-DConv Head Transposed Self-Attention (MDTA)
  • OverlapPatchEmbed class
  • Comprehensive unit tests for all new components

The implementation follows MONAI's coding patterns and includes performance validations against native PyTorch operations where applicable.

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.

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.

Looks good overall but I had a few inline comments, and we should have full docstrings everywhere appropriate. For any classes meant for general purpose use (ie. not just by Restormer) please ensure they have docstring descriptions for the arguments (at the very least for constructor args). Thanks!

@phisanti phisanti requested a review from aylward March 1, 2025 10:05
@phisanti
Copy link
Contributor Author

phisanti commented Mar 1, 2025

Hi @phisanti We can look at the issues with the tests, no problem. I think I should let you figure out the mixup with branches and update your tests first then we can have a look at what the issue is. We should avoid just ignoring tests as much as possible, but if you need einops for example you can use @skipUnless(has_einops, "Requires einops") as a test method decorator. If that isn't working for you we can figure it out together.

Hi @ericspod, I finally managed to find some time to do the merge and be up-to-date with main. Now, in regards with the unit test, I placed them all in their appropriate folders mirroring the structure within source:

/tests/networks/utils/test_pixelunshuffle.py
/tests/networks/blocks/test_CABlock.py
/tests/networks/blocks/test_downsample_block.py
/tests/networks/nets/test_restormer.py

I also noticed that I have to move the import of SkipIfBeforePyTorchVersion in test_CABlock.py from tests.utils to tests.test_utils. I have placed the statement einops, has_einops = optional_import("einops") plus @skipUnless(has_einops, "Requires einops"), however, it seems some of the checks still fail. Thus, any help will be welcome, I believe the required changes are quite minimal.

phisanti and others added 12 commits March 1, 2025 17:37
… <[email protected]>,

I, Cano-Muniz, Santiago <[email protected]>, hereby add my Signed-off-by to this commit: 55da640
I, Cano-Muniz, Santiago <[email protected]>, hereby add my Signed-off-by to this commit: 3c2dbc6

Signed-off-by: Cano-Muniz, Santiago <[email protected]>
…ition, solve DCO:

DCO Remediation Commit for tisalon <[email protected]>

I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 8faa5da
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 091887b
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 5d162d0
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: f520e99
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 39d1edf
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 5b3d4e1
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 1683b14
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 232be1c
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: d1df8e6
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 78ce56b
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: ce15886
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 30fad17
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 529e90b
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: c109029
I, tisalon <[email protected]>, hereby add my Signed-off-by to this commit: 19c30f7

Signed-off-by: tisalon <[email protected]>
…ses based on einops availability, and solve last DCO issue:

I, Cano-Muniz, Santiago <[email protected]>, hereby add my Signed-off-by to this commit: f17e06e

Signed-off-by: Cano-Muniz, Santiago <[email protected]>
…less lower so that it interpreted first).

Signed-off-by: Cano-Muniz, Santiago <[email protected]>
…le.py, restormer.py and test_downsample_block.py.

Signed-off-by: Cano-Muniz, Santiago <[email protected]>
@phisanti
Copy link
Contributor Author

@ericspod and @aylward, Thanks for your patience with this PR. It seems like the unit tests were creating a barrier to moving forward. The solution was actually quite simple - just needed to change the order of the SkipUnless decorator (since lower decorators execute first). You can now see that all the checks pass (except DCO due to some unsigned commits by other authors)

Does this address all the concerns? How would you like me to finalize these changes before the merge?

@ericspod
Copy link
Member

Hi @phisanti We've done some updates to requirements and our testing process. I've updated your branch now so we'll see if things get broken, but if not what's left is to sort the DCO issue. The instruction you're given should be correct for doing a remedial commit, but if not we can work that out.

@phisanti
Copy link
Contributor Author

Hi @ericspod, I'm glad to hear the code passes the checks! Regarding the DCO issue, I notice it requires specific remediation commits from Eric Kerfoot and Yiheng Wang. Since I'm not either of these authors, I don't think I can directly address this DCO requirement myself. What would be the appropriate next steps here to get this resolved?

@ericspod
Copy link
Member

I will just mark the DCO as passed, this gets confused sometimes by commiting suggestions and other changes. @aylward your review is marked as "Request Changes", is there anything more you wanted to address? @KumoLiu Please trigger blossom when you can.

@KumoLiu
Copy link
Contributor

KumoLiu commented Mar 20, 2025

/build

@ericspod ericspod dismissed aylward’s stale review March 24, 2025 13:09

I think in the interest of progress we should merge this now, we can come back to further comment and amendments if we need of course.

@ericspod ericspod enabled auto-merge (squash) March 26, 2025 13:33
@KumoLiu
Copy link
Contributor

KumoLiu commented Mar 27, 2025

/build

@ericspod ericspod merged commit bfcb318 into Project-MONAI:dev Mar 27, 2025
26 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.

6 participants