fix(metrics): Corrected dimension count in MS-SSIM error message for 3D images.#8685
Conversation
📝 WalkthroughWalkthroughCorrected a dimensionality validation error message in the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
I, Akshat Sinha <akshatsinhasramhardy@gmail.com>, hereby add my Signed-off-by to this commit: 72aaf1c Signed-off-by: Akshat Sinha <akshatsinhasramhardy@gmail.com>
There was a problem hiding this comment.
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/metrics/regression.py (1)
616-638:⚠️ Potential issue | 🟡 Minor
ssimmay be unbound ifweightsis empty.
ssimis assigned only inside the loop body (line 618). If a caller passesweights=(),len(weights_tensor) == 0, the loop never runs, andssimat line 636 raisesUnboundLocalError.🛡️ Proposed guard
+ if len(weights_tensor) == 0: + raise ValueError("`weights` must not be empty.") + multiscale_list: list[torch.Tensor] = [] for _ in range(len(weights_tensor)):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/regression.py` around lines 616 - 638, The loop uses ssim set inside compute_ssim_and_cs, so if weights_tensor is empty ssim is unbound; add a guard before the multiscale loop: if len(weights_tensor) == 0 call compute_ssim_and_cs(y_pred, y, spatial_dims, data_range, kernel_type, kernel_size, kernel_sigma, k1, k2) once to produce ssim (and cs), compute the same reductions (ssim.view(...).mean(1) and relu on that value) and set multiscale_list accordingly (then stack to multiscale_list_tensor) or alternatively raise a clear ValueError; update references to ssim, multiscale_list, compute_ssim_and_cs, avg_pool and multiscale_list_tensor so code never accesses ssim when weights_tensor is empty.
🧹 Nitpick comments (2)
monai/metrics/regression.py (2)
566-582:compute_ms_ssimdocstring is missing aReturns:section.Per coding guidelines, docstrings must describe return values. The function returns
ms_ssim_per_batchbut the docstring has noReturns:block.♻️ Proposed addition
Raises: ValueError: when `y_pred` is not a 2D or 3D image. + Returns: + ms_ssim_per_batch: MS-SSIM scores per batch element, shape ``(B, 1)``. """As per coding guidelines, "
**/*.py: Docstrings should be present for all definitions which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/regression.py` around lines 566 - 582, The compute_ms_ssim docstring is missing a Returns section; add a "Returns:" block describing the returned value ms_ssim_per_batch (its type, shape and meaning) and any conditions (e.g., per-batch MS-SSIM scores as a tensor of shape [B] or [B, C] depending on implementation), and note the unit/range (e.g., values in [0,1]) so callers know what to expect; update the compute_ms_ssim docstring accordingly to reference ms_ssim_per_batch and include the return type and brief description.
249-253:compute_mean_error_metricshas no docstring.Per coding guidelines, all definitions require docstrings.
♻️ Proposed docstring
def compute_mean_error_metrics(y_pred: torch.Tensor, y: torch.Tensor, func: Callable) -> torch.Tensor: + """Compute a per-batch mean error metric. + + Args: + y_pred: predicted tensor, shape ``(B, C, ...)``. + y: ground truth tensor, same shape as ``y_pred``. + func: element-wise error function applied to ``(y - y_pred)``. + + Returns: + Mean error value per batch element, shape ``(B, 1)``. + """ # reducing in only channel + spatial dimensions (not batch)As per coding guidelines, "
**/*.py: Docstrings should be present for all definitions which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/regression.py` around lines 249 - 253, Add a Google-style docstring to compute_mean_error_metrics describing purpose, parameters, return value, and any exceptions: explain that the function computes per-sample mean of a provided error function applied to (y - y_pred), that y_pred and y are torch.Tensors with batch as dim0 and channel+spatial dims flattened via flt (partial(torch.flatten, start_dim=1)), and func is a Callable applied elementwise; document the returned torch.Tensor shape (per-batch mean with keepdim=True), units/semantics, and note any assumptions or raised errors (e.g., mismatched shapes or non-tensor inputs) so callers know preconditions.
🤖 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/metrics/regression.py`:
- Around line 590-594: Rename the three test methods so unittest discovers and
runs them: change input_ill_input_shape2d to test_input_ill_input_shape2d,
input_ill_input_shape3d to test_input_ill_input_shape3d, and small_inputs to
test_small_inputs; ensure test_input_ill_input_shape2d constructs a 4D y_pred
and calls the metric with spatial_dims=3 so the ValueError path in regression.py
(the block checking "if spatial_dims == 3 and dims != 5") is exercised and
assert the raised error message matches the expected text.
---
Outside diff comments:
In `@monai/metrics/regression.py`:
- Around line 616-638: The loop uses ssim set inside compute_ssim_and_cs, so if
weights_tensor is empty ssim is unbound; add a guard before the multiscale loop:
if len(weights_tensor) == 0 call compute_ssim_and_cs(y_pred, y, spatial_dims,
data_range, kernel_type, kernel_size, kernel_sigma, k1, k2) once to produce ssim
(and cs), compute the same reductions (ssim.view(...).mean(1) and relu on that
value) and set multiscale_list accordingly (then stack to
multiscale_list_tensor) or alternatively raise a clear ValueError; update
references to ssim, multiscale_list, compute_ssim_and_cs, avg_pool and
multiscale_list_tensor so code never accesses ssim when weights_tensor is empty.
---
Nitpick comments:
In `@monai/metrics/regression.py`:
- Around line 566-582: The compute_ms_ssim docstring is missing a Returns
section; add a "Returns:" block describing the returned value ms_ssim_per_batch
(its type, shape and meaning) and any conditions (e.g., per-batch MS-SSIM scores
as a tensor of shape [B] or [B, C] depending on implementation), and note the
unit/range (e.g., values in [0,1]) so callers know what to expect; update the
compute_ms_ssim docstring accordingly to reference ms_ssim_per_batch and include
the return type and brief description.
- Around line 249-253: Add a Google-style docstring to
compute_mean_error_metrics describing purpose, parameters, return value, and any
exceptions: explain that the function computes per-sample mean of a provided
error function applied to (y - y_pred), that y_pred and y are torch.Tensors with
batch as dim0 and channel+spatial dims flattened via flt (partial(torch.flatten,
start_dim=1)), and func is a Callable applied elementwise; document the returned
torch.Tensor shape (per-batch mean with keepdim=True), units/semantics, and note
any assumptions or raised errors (e.g., mismatched shapes or non-tensor inputs)
so callers know preconditions.
…3D images. (Project-MONAI#8685) Fixes # . ### Description A few sentences describing the changes proposed in this pull request. ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] 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. --------- Signed-off-by: Akshat Sinha <akshatsinhasramhardy@gmail.com>
…3D images. (Project-MONAI#8685) Fixes # . ### Description A few sentences describing the changes proposed in this pull request. ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] 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. --------- Signed-off-by: Akshat Sinha <akshatsinhasramhardy@gmail.com>
Fixes # .
Description
A few sentences describing the changes proposed in this pull request.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.