Add spatial shape constraint docs and validation for DiNTS (#6771)#8827
Open
Cado87 wants to merge 4 commits intoProject-MONAI:devfrom
Open
Add spatial shape constraint docs and validation for DiNTS (#6771)#8827Cado87 wants to merge 4 commits intoProject-MONAI:devfrom
Cado87 wants to merge 4 commits intoProject-MONAI:devfrom
Conversation
…ONAI#6771) Signed-off-by: Adrian Caderno <adriancaderno@gmail.com>
Contributor
📝 WalkthroughWalkthroughAdded runtime input-size validation to DiNTS: the forward path now calls a new private Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/networks/nets/dints.py (1)
493-504: AddReturnssections in modified docstrings for guideline compliance.Both modified methods document args/raises but omit return-value sections.
As per coding guidelines, "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."
Also applies to: 520-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/networks/nets/dints.py` around lines 493 - 504, The docstring for _check_input_size (and the other modified method immediately following it) is missing a Returns section; update both docstrings to include a Returns entry describing the return value (e.g., "None: if validation succeeds" or the actual return type) and a brief description, following Google-style docstring format so each function documents args, returns, and raises; ensure you edit the _check_input_size docstring and the subsequent modified method's docstring to add the Returns section accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/networks/nets/dints.py`:
- Line 505: The validation uses self.dints_space.use_downsample while the
constructor uses the use_downsample argument, allowing divergence; ensure both
use the same source by assigning the constructor argument into the config before
any validation or stem construction (e.g., set self.dints_space.use_downsample =
use_downsample early in DiNTS.__init__), or alternatively change
_check_input_size() to read the use_downsample argument passed into __init__;
update the places computing factor (currently using
self.dints_space.use_downsample) and stem construction to reference the unified
source so _check_input_size, DiNTS.__init__, and factor calculation are
consistent.
---
Nitpick comments:
In `@monai/networks/nets/dints.py`:
- Around line 493-504: The docstring for _check_input_size (and the other
modified method immediately following it) is missing a Returns section; update
both docstrings to include a Returns entry describing the return value (e.g.,
"None: if validation succeeds" or the actual return type) and a brief
description, following Google-style docstring format so each function documents
args, returns, and raises; ensure you edit the _check_input_size docstring and
the subsequent modified method's docstring to add the Returns section
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 648f35b7-1f1e-4150-8195-53b174ddf38a
📒 Files selected for processing (2)
monai/networks/nets/dints.pytests/networks/nets/test_dints_network.py
| Raises: | ||
| ValueError: if any spatial dimension is not divisible by the required factor. | ||
| """ | ||
| factor = 2 ** (self.num_depths + int(self.dints_space.use_downsample)) |
Contributor
There was a problem hiding this comment.
Unify use_downsample source to avoid inconsistent validation.
_check_input_size() uses self.dints_space.use_downsample, but stem construction in DiNTS.__init__ uses the use_downsample argument. If these differ, validation and actual network path can diverge.
Proposed fix
class DiNTS(nn.Module):
@@
def __init__(
@@
use_downsample: bool = True,
@@
):
super().__init__()
+ if hasattr(dints_space, "use_downsample") and dints_space.use_downsample != use_downsample:
+ raise ValueError(
+ f"DiNTS.use_downsample ({use_downsample}) must match dints_space.use_downsample "
+ f"({dints_space.use_downsample})."
+ )
+ self.use_downsample = use_downsample
@@
def _check_input_size(self, spatial_shape):
@@
- factor = 2 ** (self.num_depths + int(self.dints_space.use_downsample))
+ factor = 2 ** (self.num_depths + int(self.use_downsample))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/networks/nets/dints.py` at line 505, The validation uses
self.dints_space.use_downsample while the constructor uses the use_downsample
argument, allowing divergence; ensure both use the same source by assigning the
constructor argument into the config before any validation or stem construction
(e.g., set self.dints_space.use_downsample = use_downsample early in
DiNTS.__init__), or alternatively change _check_input_size() to read the
use_downsample argument passed into __init__; update the places computing factor
(currently using self.dints_space.use_downsample) and stem construction to
reference the unified source so _check_input_size, DiNTS.__init__, and factor
calculation are consistent.
Signed-off-by: Adrian Caderno <adriancaderno@gmail.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
monai/networks/nets/dints.py (1)
506-507:⚠️ Potential issue | 🟠 MajorUnify
use_downsamplesource for validation and stem construction.At Line 506,
_check_input_size()usesself.dints_space.use_downsample, but stem construction uses the constructoruse_downsample(Lines 414 and 457). If those diverge, validation can check the wrong factor.Suggested fix
class DiNTS(nn.Module): @@ def __init__( @@ use_downsample: bool = True, @@ ): super().__init__() + if hasattr(dints_space, "use_downsample") and dints_space.use_downsample != use_downsample: + raise ValueError( + f"DiNTS.use_downsample ({use_downsample}) must match " + f"dints_space.use_downsample ({dints_space.use_downsample})." + ) + self.use_downsample = use_downsample @@ def _check_input_size(self, spatial_shape): @@ - factor = 2 ** (self.num_depths + int(self.dints_space.use_downsample)) + factor = 2 ** (self.num_depths + int(self.use_downsample))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/networks/nets/dints.py` around lines 506 - 507, The validation in _check_input_size currently reads self.dints_space.use_downsample but stem construction and the constructor use the parameter/attribute use_downsample; change the validation to reference the same source (use self.use_downsample) so the downsample flag is consistent. Locate _check_input_size and replace any use of self.dints_space.use_downsample with the class attribute/constructor param (self.use_downsample), ensuring factor computation and wrong_dims calculation use that unified flag; also ensure the constructor sets self.use_downsample if not already.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@monai/networks/nets/dints.py`:
- Around line 506-507: The validation in _check_input_size currently reads
self.dints_space.use_downsample but stem construction and the constructor use
the parameter/attribute use_downsample; change the validation to reference the
same source (use self.use_downsample) so the downsample flag is consistent.
Locate _check_input_size and replace any use of self.dints_space.use_downsample
with the class attribute/constructor param (self.use_downsample), ensuring
factor computation and wrong_dims calculation use that unified flag; also ensure
the constructor sets self.use_downsample if not already.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f620ceee-2b1f-448c-a509-7ec30c75518e
📒 Files selected for processing (2)
monai/networks/nets/dints.pytests/networks/nets/test_dints_network.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/networks/nets/test_dints_network.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6771 .
Description
Adds spatial shape constraint documentation and runtime validation for
DiNTS.Each input spatial dimension must be divisible by
2 ** (num_depths + int(use_downsample)).Previously only a
print()warning existed inTopologyConstruction.__init__— no error was raised.This PR adds
_check_input_size()toDiNTS, calls it inforward(), updates the class docstringwith a
Spatial Shape Constraintssection, and adds tests for both 3D and 2D invalid inputs.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.