-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Diffusion Model Encoder has an output layer set in forward method and this leads to problems #8578
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
Conversation
Co-authored-by: Virginia Fernandez <[email protected]>
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/networks/nets/diffusion_model_unet.py (1)
1989-1989
: Off-by-one error in final block checkThe condition incorrectly uses
len(channels)
instead oflen(channels) - 1
, causing incorrect downsampling behavior.- is_final_block = i == len(channels) # - 1 + is_final_block = i == len(channels) - 1
🧹 Nitpick comments (2)
monai/networks/nets/diffusion_model_unet.py (2)
36-39
: Unnecessary imports added
functools.reduce
andnumpy
are only used inDiffusionModelEncoder
. Consider moving these imports closer to where they're used or making them conditional imports to avoid unnecessary dependencies for users ofDiffusionModelUNet
.
1887-1887
: Missinginput_shape
in docstringThe new
input_shape
parameter needs documentation in the docstring.Add to the Args section:
Args: spatial_dims: number of spatial dimensions. in_channels: number of input channels. out_channels: number of output channels. + input_shape: spatial shape of the input (without batch and channel dims). num_res_blocks: number of residual blocks (see _ResnetBlock) per level.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base
setting
📒 Files selected for processing (1)
monai/networks/nets/diffusion_model_unet.py
(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/networks/nets/diffusion_model_unet.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: build-docs
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.10)
🔇 Additional comments (3)
monai/networks/nets/diffusion_model_unet.py (3)
2014-2017
: Ceil division maintains odd dimensions correctlyThe spatial dimension calculation using
np.ceil
properly handles odd-sized inputs as intended.
2019-2021
: Output layer correctly moved to initThe MLP head is now properly instantiated during initialization, resolving the device and optimizer issues.
1543-1543
: Document or remove the newinput_shape
parameterConstructor adds
input_shape
(monai/networks/nets/diffusion_model_unet.py ≈ line 1543) but verification scripts returned no output — manually confirm whetherinput_shape
is referenced anywhere; if unused remove it, otherwise wire it into the class and add/update its parameter docs and tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/networks/nets/diffusion_model_unet.py (1)
1989-1989
: Bug: Off-by-one error in final block checkThis will cause an index out of bounds error. The condition should be
i == len(channels) - 1
.- is_final_block = i == len(channels) # - 1 + is_final_block = i == len(channels) - 1
🧹 Nitpick comments (2)
monai/networks/nets/diffusion_model_unet.py (2)
1907-1907
: Document the expected format of input_shapeAdd clarification about whether this should include batch/channel dimensions.
- input_shape: spatial shape of the input (without batch and channel dims). + input_shape: spatial shape of the input (H, W) for 2D or (H, W, D) for 3D, excluding batch and channel dims.
2019-2021
: Type annotation inconsistency
self.out
should not beOptional
since it's always initialized.- self.out: Optional[nn.Module] = nn.Sequential( + self.out = nn.Sequential(
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base
setting
📒 Files selected for processing (1)
monai/networks/nets/diffusion_model_unet.py
(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/networks/nets/diffusion_model_unet.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.10)
….ac.uk> I, Virginia Fernandez <[email protected]>, hereby add my Signed-off-by to this commit: 537945f I, Virginia Fernandez <[email protected]>, hereby add my Signed-off-by to this commit: d5856ec I, Virginia Fernandez <[email protected]>, hereby add my Signed-off-by to this commit: c9bb8f7 I, Virginia Fernandez <[email protected]>, hereby add my Signed-off-by to this commit: cbf5dc5 Signed-off-by: Virginia Fernandez <[email protected]>
Signed-off-by: Virginia Fernandez <[email protected]>
….ac.uk> I, Virginia Fernandez <[email protected]>, hereby add my Signed-off-by to this commit: 537945f I, Virginia Fernandez <[email protected]>, hereby add my Signed-off-by to this commit: d5856ec I, Virginia Fernandez <[email protected]>, hereby add my Signed-off-by to this commit: c9bb8f7 I, Virginia Fernandez <[email protected]>, hereby add my Signed-off-by to this commit: cbf5dc5 Signed-off-by: Virginia Fernandez <[email protected]>
Signed-off-by: Virginia Fernandez <[email protected]>
/build |
Fixes #8577 .
Description
This pull request sets the out layer of DiffusionModelEncoder in the init method. This requires the inclusion of input_shape parameter in the init method to calculate the input dimension to the last linear layer.
The output spatial shape derivation is a bit baroque, but allows for the otherwise not very pleasant odd spatial dimensions at input.
Types of changes
./runtests.sh --quick --unittests --disttests
.