Fix #8366: Add strict shape validation to sliding_window_inference #8645
Conversation
mattlin1124
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
WalkthroughDocstring in sliding_window_inference updated to explicitly require channel-first inputs (NCHW / NCDHW) and to advise converting NHWC/NDHWC upstream. Added strict shape validation: when Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
monai/inferers/utils.py (1)
177-177: Duplicate calculation ofnum_spatial_dims.
num_spatial_dimsis computed on line 177 and again on line 197. Remove the duplicate.Apply this diff:
""" num_spatial_dims = len(inputs.shape) - 2 # Only perform strict shape validation if roi_size is a sequence (explicit dimensions). # If roi_size is an integer, it is broadcast to all dimensions, so we cannot # infer the expected dimensionality to enforce a strict check here. if not isinstance(roi_size, int): roi_dims = len(roi_size) if num_spatial_dims != roi_dims: raise ValueError( f"inputs must have {roi_dims + 2} dimensions for {roi_dims}D roi_size " f"(Batch, Channel, {', '.join(['Spatial'] * roi_dims)}), " f"but got inputs shape {inputs.shape}.\n" "If you have channel-last data (e.g. B, D, H, W, C), please use " "monai.transforms.EnsureChannelFirst or EnsureChannelFirstd upstream." ) # ----------------------------------------------------------------- # ---- Strict validation: do NOT guess or permute layouts ---- if isinstance(inputs, torch.Tensor): assert_channel_first(inputs, "inputs") buffered = buffer_steps is not None and buffer_steps > 0 - num_spatial_dims = len(inputs.shape) - 2 if buffered:Also applies to: 197-197
tests/inferers/test_sliding_window_inference.py (1)
375-394: Consider testing edge cases.Current test covers dimensional mismatches. Consider adding:
- Valid 5D input (should pass)
roi_sizeas int (should skip strict validation)- MetaTensor input
Example additional test cases:
# Valid 5D input should pass inputs_5d = torch.randn((1, 3, 16, 16, 16), device=device) result = sliding_window_inference(inputs_5d, roi_size, sw_batch_size, predictor) self.assertEqual(result.shape, inputs_5d.shape) # roi_size as int should skip strict validation (preserves existing behavior) inputs_4d_int_roi = torch.randn((1, 3, 16, 16), device=device) result = sliding_window_inference(inputs_4d_int_roi, 8, sw_batch_size, predictor) self.assertIsNotNone(result)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/inferers/utils.py(2 hunks)tests/inferers/test_sliding_window_inference.py(1 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:
tests/inferers/test_sliding_window_inference.pymonai/inferers/utils.py
🧬 Code graph analysis (1)
tests/inferers/test_sliding_window_inference.py (1)
monai/inferers/utils.py (1)
sliding_window_inference(81-390)
🪛 Ruff (0.14.6)
monai/inferers/utils.py
47-47: Undefined name Optional
(F821)
72-76: Avoid specifying long messages outside the exception class
(TRY003)
185-191: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: build-docs
🔇 Additional comments (3)
monai/inferers/utils.py (2)
173-174: LGTM!Clear documentation of channel-first requirement with actionable guidance.
177-196: Strict validation logic is sound.Correctly validates dimensionality mismatch and provides clear error guidance. The check for
isinstance(roi_size, int)appropriately skips validation for broadcasted cases.tests/inferers/test_sliding_window_inference.py (1)
375-394: Test correctly validates dimensional mismatch detection.Both test cases properly verify that inputs with incorrect dimensions raise ValueError with the expected message.
mattlin1124
force-pushed
the
fix-8366-strict-validation
branch
from
7800890 to
4aa75d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/inferers/utils.py (1)
190-207: Strictroi_sizevs input dimensionality check looks good; consider minor robustness tweaks.The new block correctly enforces that a sequence
roi_sizewithkspatial dims is only accepted for inputs withkspatial dims (k + 2 total), and the error message clearly points users toEnsureChannelFirst[d]. Two minor, optional points:
isinstance(roi_size, int)will treat NumPy integer scalars as non-int and then calllen(roi_size), which will fail; if you expect such inputs in the wild,numbers.Integral(or similar) would make this more robust.- Ruff’s TRY003 warning on the long
ValueErrormessage could be silenced by factoring the message into a localmsgvariable, but that’s purely stylistic.Otherwise the logic and placement (before padding and window setup) are sound.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/inferers/utils.py(3 hunks)tests/inferers/test_sliding_window_inference.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/inferers/test_sliding_window_inference.py
🧰 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/inferers/utils.py
🧬 Code graph analysis (1)
monai/inferers/utils.py (1)
monai/utils/module.py (1)
optional_import(315-445)
🪛 Ruff (0.14.6)
monai/inferers/utils.py
83-87: Avoid specifying long messages outside the exception class
(TRY003)
196-202: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: packaging
- GitHub Check: build-docs
🔇 Additional comments (2)
monai/inferers/utils.py (2)
16-36: Import ofOptionalis correct and resolves the previous undefined-name.Type hints below use
Optional; this import is appropriate and fixes the Ruff F821 warning without side effects.
184-186: Docstring note clarifies channel-first requirement and is aligned with the new error messaging.The added Note lines make the NCHW/NCDHW expectation and the
EnsureChannelFirst/EnsureChannelFirstdrecommendation explicit, which matches the newValueErrortext below.
mattlin1124
force-pushed
the
fix-8366-strict-validation
branch
from
c95babd to
aada4f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
monai/inferers/utils.py (1)
44-90:assert_channel_firstlayout enforcement andallow_binary_two_channelbehavior are inconsistent with intent.Two issues here:
No real layout enforcement when
num_classes is None.
- For 4D/5D tensors, if
num_classesis left asNone(as insliding_window_inference(inputs, ...)), this function never raises. It only returns early for non‑tensors/other ndims and otherwise performs no checks.- Docstring and the trailing comment (“Enforce channel-first layout …”, “we simply require channel at dim=1”) overstate what is enforced. As used in
sliding_window_inference, this helper is effectively a no‑op and does not reject NHWC/NDHWC inputs; only the new roi_size/ndim check adds validation.
allow_binary_two_channelis a no‑op.
- When
num_classes == 2,ok = (c == num_classes)already acceptsC == 2. The extra branch
if allow_binary_two_channel and num_classes == 2: ok = ok or (c == 2)
never expands the accepted cases, so the flag has no effect and adds API surface without behavior.I’d suggest either simplifying the API (drop
allow_binary_two_channelfor now) or making both the flag and the docstring match the actual behavior. For example, if the goal is to support binary tasks that may be encoded as 1‑channel or 2‑channel, something like this would make the flag meaningful and keep the comments honest:@@ -def assert_channel_first( - t: torch.Tensor, - name: str, - num_classes: Optional[int] = None, - allow_binary_two_channel: bool = False, -) -> None: +def assert_channel_first( + t: torch.Tensor, + name: str, + num_classes: Optional[int] = None, + allow_binary_two_channel: bool = False, +) -> None: @@ - if num_classes is not None: - ok = (c == num_classes) or (num_classes == 1 and c == 1) - if allow_binary_two_channel and num_classes == 2: - ok = ok or (c == 2) + if num_classes is not None: + ok = c == num_classes + # For binary tasks, optionally accept either 1‑channel or C==num_classes. + if allow_binary_two_channel and num_classes == 2: + ok = ok or (c == 1) if not ok: @@ - # No guessing when num_classes is None; we simply require channel at dim=1. - # If callers provided NHWC/NDHWC, they must convert upstream. + # When num_classes is None we only check dimensionality; callers are + # responsible for ensuring channel-first layout (NCHW/NCDHW) upstream.Alternatively, if you don’t need the flag yet, dropping
allow_binary_two_channeland updating the doc/comment accordingly would keep the helper minimal and avoid confusion.
🧹 Nitpick comments (1)
monai/inferers/utils.py (1)
183-207: New roi_size vs ndim validation is good; consider whereassert_channel_firstruns and long error strings.The strict check
- computes
num_spatial_dims = len(inputs.shape) - 2,- compares it to
len(roi_size)whenroi_sizeis a sequence, and- raises a clear
ValueErrorwith guidance to useEnsureChannelFirst[d],which aligns with the PR goal of explicit dimensionality validation without guessing. Skipping the check when
roi_sizeis anintalso preserves the existing 1D/broadcast behavior.Two small follow‑ups to consider:
- Placement of
assert_channel_first. Right now it runs only wheninputsis atorch.Tensor, beforeconvert_data_type. If callers ever pass numpy arrays (even though the type hint is Tensor/MetaTensor), those inputs get converted later but never go throughassert_channel_first. If you want uniform enforcement, you could move the call to just afterconvert_data_type(...)so the check always sees a tensor.- Long error messages (TRY003). Both the new
ValueErrorhere and the one inassert_channel_firstare long multi‑line strings. If you want to satisfy Ruff’s TRY003, you could factor these into a small helper or custom exception instead of embedding the full text in theraise.These are non‑blocking and can be deferred if you prefer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/inferers/utils.py(3 hunks)tests/inferers/test_sliding_window_inference.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/inferers/test_sliding_window_inference.py
🧰 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/inferers/utils.py
🧬 Code graph analysis (1)
monai/inferers/utils.py (1)
monai/utils/module.py (1)
optional_import(315-445)
🪛 Ruff (0.14.6)
monai/inferers/utils.py
83-87: Avoid specifying long messages outside the exception class
(TRY003)
196-202: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (windows-latest)
🔇 Additional comments (1)
monai/inferers/utils.py (1)
16-16: Import ofOptionalis correct and resolves the undefined name.No issues here; this matches the new type hints.
mattlin1124
force-pushed
the
fix-8366-strict-validation
branch
from
897b8e1 to
8491fdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/inferers/utils.py(2 hunks)tests/inferers/test_sliding_window_inference.py(1 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/inferers/utils.pytests/inferers/test_sliding_window_inference.py
🧬 Code graph analysis (2)
monai/inferers/utils.py (1)
monai/utils/module.py (1)
optional_import(315-445)
tests/inferers/test_sliding_window_inference.py (1)
monai/inferers/utils.py (1)
sliding_window_inference(86-393)
🪛 Ruff (0.14.6)
monai/inferers/utils.py
37-37: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
44-44: Unexpected indentation
(invalid-syntax)
⏰ 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). (13)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
🔇 Additional comments (3)
monai/inferers/utils.py (2)
178-179: LGTM: Clear documentation of channel-first requirement.The updated Note appropriately directs users to convert channel-last data upstream.
183-199: LGTM: Strict validation correctly implemented.The dimensionality check properly enforces that input shape matches roi_size dimensions, with appropriate skipping for integer roi_size. Error message clearly guides users to upstream conversion.
tests/inferers/test_sliding_window_inference.py (1)
375-394: Unable to complete verification due to repository access failure. Based on the provided code snippet alone, the test methodtest_strict_shape_validationis shown once (lines 375-394) and the test logic is sound—it correctly validates that 4D and 3D inputs raiseValueErrorwith the expected message when a 3Droi_size(requiring 5D input) is provided. However, without access to the full test file, I cannot confirm whether duplication exists elsewhere or verify test coverage completeness. Additionally, the test method lacks a docstring, which violates the PEP8 guideline requiring docstrings for all definitions.
monai/inferers/utils.py
Outdated
|
|
||
| """ | ||
| Enforce channel-first layout (NCHW/NCDHW) without guessing. | ||
| Args: | ||
| t: Input tensor to validate. | ||
| name: Name of the tensor for error messages. | ||
| num_classes: Optional expected channel count at dim=1. | ||
| allow_binary_two_channel: If True and num_classes==2, accept C=2. | ||
| Raises: | ||
| ValueError: If tensor shape doesn't match channel-first layout or | ||
| num_classes constraint. | ||
| Note: | ||
| - Accepts only 4D (NCHW) or 5D (NCDHW) tensors with channel at dim=1. | ||
| - Non-tensors and tensors with other dimensionalities are silently ignored. | ||
| - Users must apply EnsureChannelFirst/EnsureChannelFirstd upstream for | ||
| channel-last data. | ||
| """ | ||
| if not isinstance(t, torch.Tensor): | ||
| return | ||
| if t.ndim not in (4, 5): | ||
| return | ||
|
|
||
| c = int(t.shape[1]) | ||
| layout = "NCHW" if t.ndim == 4 else "NCDHW" | ||
| layout_last = "NHWC" if t.ndim == 4 else "NDHWC" | ||
|
|
||
| if num_classes is not None: | ||
| ok = (c == num_classes) or (num_classes == 1 and c == 1) | ||
| if allow_binary_two_channel and num_classes == 2: | ||
| ok = ok or (c == 2) | ||
| if not ok: | ||
| raise ValueError( | ||
| f"{name}: expected {layout} with C(dim=1)==num_classes, " | ||
| f"but got shape={tuple(t.shape)} (C={c}) and num_classes={num_classes}. " | ||
| f"If your data is {layout_last}, please apply EnsureChannelFirst/EnsureChannelFirstd upstream." | ||
| ) | ||
| # No guessing when num_classes is None; we simply require channel at dim=1. | ||
| # If callers provided NHWC/NDHWC, they must convert upstream. | ||
|
|
There was a problem hiding this comment.
Missing function signature and function is never called.
The function definition line for assert_channel_first is missing between lines 43 and 44. Additionally, this function is never invoked in sliding_window_inference, contradicting the AI summary.
Add the function signature at line 43:
+def assert_channel_first(t: torch.Tensor, name: str, num_classes: Optional[int] | None = None, allow_binary_two_channel: bool = False) -> None:
"""Note: You'll also need to import Optional from typing if using the Optional[int] syntax, or use int | None as shown above for Python 3.10+.
Then, determine where in sliding_window_inference this function should be called to enforce channel-first layout, or remove it if unused.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.6)
44-44: Unexpected indentation
(invalid-syntax)
mattlin1124
force-pushed
the
fix-8366-strict-validation
branch
from
8491fdb to
df0af5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monai/inferers/utils.py (1)
155-156: Orphaned comment after validation block.Comment appears after validation ends (line 153), making it unclear what it refers to. Either move it before line 141 as a section header or remove it since lines 141-143 already explain the validation approach.
Apply this diff to remove the orphaned comment:
- # ----------------------------------------------------------------- - # ---- Strict validation: do NOT guess or permute layouts ---- -Or move it before the validation block:
+ # ----------------------------------------------------------------- + # ---- Strict validation: do NOT guess or permute layouts ---- + # Only perform strict shape validation if roi_size is a sequence (explicit dimensions).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/inferers/utils.py(2 hunks)tests/inferers/test_sliding_window_inference.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/inferers/test_sliding_window_inference.py
🧰 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/inferers/utils.py
🪛 Ruff (0.14.6)
monai/inferers/utils.py
147-153: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (windows-latest)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (macOS-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-py3 (3.12)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.11)
🔇 Additional comments (4)
monai/inferers/utils.py (4)
37-37: LGTM.tqdm import follows MONAI's optional_import pattern and is used on line 238.
135-136: LGTM.Docstring clarifies channel-first requirement and provides clear guidance for channel-last data.
141-153: LGTM.Validation logic correctly enforces dimension matching when
roi_sizeis a sequence, skips when integer (preserving broadcasting). Verbose error message (TRY003 flag) is intentional and provides clear user guidance per PR objectives.
157-157: LGTM.Moving
bufferedinitialization after validation follows fail-fast principle - validate inputs before setup.
mattlin1124
force-pushed
the
fix-8366-strict-validation
branch
from
df0af5f to
95119e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/inferers/utils.py (1)
134-137: Document the new ValueError in the docstring (optional)Since the function now raises a
ValueErrorfor shape mismatches, consider adding a shortRaises:section to the docstring mentioning this, to stay closer to the Google-style guideline used elsewhere.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/inferers/utils.py(2 hunks)tests/inferers/test_sliding_window_inference.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/inferers/test_sliding_window_inference.py
🧰 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/inferers/utils.py
🪛 Ruff (0.14.6)
monai/inferers/utils.py
147-153: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
🔇 Additional comments (1)
monai/inferers/utils.py (1)
36-37: tqdm optional_import usage looks goodModule-level
tqdmimport viaoptional_importmatches common MONAI patterns and keeps the dependency optional; no issues here.
mattlin1124
force-pushed
the
fix-8366-strict-validation
branch
2 times, most recently
from
de91e02 to
d33e678
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monai/inferers/utils.py (1)
140-155: Validation logic is correct and well-structured.The strict shape validation properly enforces dimensionality requirements for sequence
roi_sizewhile preserving backward compatibility for integer/scalar values via theIntegralcheck. The error message is clear and directs users to the appropriate transforms.The
bufferedflag initialization at line 155 improves code clarity by making the condition explicit.Optional: Static analysis flags the error message length (TRY003). You could extract it to a constant if preferred:
_SHAPE_MISMATCH_MSG = ( "inputs must have {expected_dims} dimensions for {roi_dims}D roi_size " "(Batch, Channel, {spatial_dims}), but got inputs shape {actual_shape}.\n" "If you have channel-last data (e.g. B, D, H, W, C), please use " "monai.transforms.EnsureChannelFirst or EnsureChannelFirstd upstream." )Then use:
raise ValueError(_SHAPE_MISMATCH_MSG.format( expected_dims=roi_dims + 2, roi_dims=roi_dims, spatial_dims=', '.join(['Spatial'] * roi_dims), actual_shape=inputs.shape ))However, the current inline approach is perfectly acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/inferers/utils.py(2 hunks)tests/inferers/test_sliding_window_inference.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/inferers/test_sliding_window_inference.py
🧰 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/inferers/utils.py
🪛 Ruff (0.14.6)
monai/inferers/utils.py
147-153: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
monai/inferers/utils.py (2)
16-16: LGTM!The
Integralimport is correctly placed and necessary for the robust type checking in the validation logic below.
135-136: LGTM!The docstring clarification properly documents the channel-first requirement and provides clear guidance for handling channel-last data upstream.
mattlin1124
force-pushed
the
fix-8366-strict-validation
branch
from
d33e678 to
974c1d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/inferers/utils.py(1 hunks)tests/inferers/test_sliding_window_inference.py(1 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/inferers/utils.pytests/inferers/test_sliding_window_inference.py
🧬 Code graph analysis (1)
tests/inferers/test_sliding_window_inference.py (1)
monai/inferers/utils.py (1)
sliding_window_inference(42-347)
🪛 Ruff (0.14.6)
monai/inferers/utils.py
146-152: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
monai/inferers/utils.py (1)
133-136: LGTM!Clear documentation of the channel-first requirement.
tests/inferers/test_sliding_window_inference.py (1)
375-393: Unable to verify due to repository access limitations. Manual verification required to confirm:
- Whether an inverted condition exists in
sliding_window_inferencefunction- Whether the test assertions are correct
- Current state of validation logic
…_inference Signed-off-by: 林旻佑 <[email protected]>
mattlin1124
force-pushed
the
fix-8366-strict-validation
branch
from
974c1d9 to
1103747
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
monai/inferers/utils.py (1)
138-153: Shape validation logic is sound; consider clarifying supported roi_size types and factoring error textThe new
Sequence-based check correctly enforcesinputs.ndim == len(roi_size) + 2for tuple/list/torch.Sizeroi sizes and skips scalar/broadcasted cases, matching the PR intent. Two minor follow‑ups:
- Ensure tests cover typical non‑tuple ROI sizes you expect in the wild (e.g.,
torch.Size, possiblynp.ndarray) so thisSequencecheck aligns with caller behavior.- To satisfy Ruff’s TRY003 and keep the function lean, you might move the long error message string into a small helper or constant.
🧹 Nitpick comments (1)
monai/inferers/utils.py (1)
77-136: Docstring doesn’t document new ValueError or follow full Google-style sectionsThe function now raises a
ValueErrorfor mismatched dimensionality, but the docstring lacks aRaises:(andReturns:) section and still uses a mixed style. Consider adding properArgs,Returns, andRaisessections that explicitly mention the dimensionality check and the channel‑first expectation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/inferers/utils.py(1 hunks)tests/inferers/test_sliding_window_inference.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/inferers/test_sliding_window_inference.py
🧰 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/inferers/utils.py
🪛 Ruff (0.14.6)
monai/inferers/utils.py
146-152: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Hi @mattlin1124 thanks for this addition, I think it looks fine as an added sanity check of the dimensions but leaves figuring out channel dimension first to the user. Does this supercede #8532? If so you can close that PR if this one completely addresses the problem. My comments here are very minor.
| roi_dims = len(roi_size) | ||
| if num_spatial_dims != roi_dims: | ||
| raise ValueError( | ||
| f"inputs must have {roi_dims + 2} dimensions for {roi_dims}D roi_size " |
There was a problem hiding this comment.
| f"inputs must have {roi_dims + 2} dimensions for {roi_dims}D roi_size " | |
| f"Inputs must have {roi_dims + 2} dimensions for {roi_dims}D roi_size " |
| # Case 1: Input has fewer dimensions than expected (e.g., missing Batch or Channel) | ||
| # 3D roi_size requires 5D input (B, C, D, H, W), giving 4D here. | ||
| inputs_4d = torch.randn((1, 16, 16, 16), device=device) | ||
| with self.assertRaisesRegex(ValueError, "inputs must have 5 dimensions"): |
There was a problem hiding this comment.
| with self.assertRaisesRegex(ValueError, "inputs must have 5 dimensions"): | |
| with self.assertRaisesRegex(ValueError, "Inputs must have 5 dimensions"): |
|
|
||
| # Case 2: Input is 3D (missing Batch AND Channel) | ||
| inputs_3d = torch.randn((16, 16, 16), device=device) | ||
| with self.assertRaisesRegex(ValueError, "inputs must have 5 dimensions"): |
There was a problem hiding this comment.
| with self.assertRaisesRegex(ValueError, "inputs must have 5 dimensions"): | |
| with self.assertRaisesRegex(ValueError, "Inputs must have 5 dimensions"): |
| Args: | ||
| inputs: input image to be processed (assuming NCHW[D]) | ||
| roi_size: the spatial window size for inferences. |
There was a problem hiding this comment.
| roi_size: the spatial window size for inferences, this must be a single value or a tuple with values for each spatial dimension (eg. 2 for 2D, 3 for 3D). |
I would add a little more explanation here so it's clearer.
| If True, the signature of ``predictor`` should be ``predictor(patch_data, patch_coord, ...)``. | ||
| args: optional args to be passed to ``predictor``. | ||
| kwargs: optional keyword args to be passed to ``predictor``. | ||
There was a problem hiding this comment.
You could add a "raises" section to describe under what conditions your added exception is raised.
2 participants
Description
This PR addresses issue #8366 by implementing strict shape validation in
sliding_window_inference.Per the feedback from maintainers (@ericspod), implicit guessing (heuristics) for channel-last data has been avoided. Instead, this PR ensures that:
roi_size(e.g., must be 5D for 3Droi_size).roi_sizeis an integer (broadcasting), preventing regressions in existing 1D/broadcasting tests.ValueErroris raised if dimensions do not match, guiding users to handle channel-last data upstream usingEnsureChannelFirstorEnsureChannelFirstd.Status
monai/inferers/utils.pytests/inferers/test_sliding_window_inference.py.gitignoreTypes of changes