feat(losses/metrics): implement ignore_index support across dice and …#8757
Open
Rusheel86 wants to merge 18 commits intoProject-MONAI:devfrom
Open
feat(losses/metrics): implement ignore_index support across dice and …#8757Rusheel86 wants to merge 18 commits intoProject-MONAI:devfrom
Rusheel86 wants to merge 18 commits intoProject-MONAI:devfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional ignore_index: int | None parameter to many loss and metric classes (DiceLoss, FocalLoss, TverskyLoss, unified focal losses, DiceMetric, MeanIoU, GeneralizedDiceScore, ConfusionMatrixMetric, Hausdorff/SurfaceDistance/SurfaceDice) and related helpers. Each constructor stores self.ignore_index and forward/compute paths build per-batch/per-voxel masks (channel-aware or sentinel-aware) to mask input/target before reductions. Utilities in metrics/utils (ignore_index_mask, mask-aware edge/distance routines) were introduced/extended. Tests for losses and metrics validating ignore_index behavior were added. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
Rusheel86
force-pushed
the
feat-ignore-index-support
branch
6 times, most recently
from
7177b8d to
b74ef93
Compare
…ated unit tests Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Rusheel86
force-pushed
the
feat-ignore-index-support
branch
from
b74ef93 to
f2caaf8
Compare
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
I, Rusheel Sharma <rusheelhere@gmail.com>, hereby add my Signed-off-by to this commit: d075009 Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
monai/losses/focal_loss.py (1)
150-154:⚠️ Potential issue | 🔴 Critical
ignore_indexis derived from the wrong target representation.The comparison happens after any
one_hotconversion, so it only works for sentinel-filled tensors. For documented one-hot targets, class IDs are gone andignore_indexnever identifies the ignored voxels. Keep the pre-conversion labels for masking, and fall back to the ignored class channel when the target is already one-hot.Also applies to: 167-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/losses/focal_loss.py` around lines 150 - 154, The ignore_index mask is being computed from the post-conversion target (after one_hot), so when users pass one-hot targets the class IDs are lost and ignored voxels aren’t detected; fix by saving the original target (e.g., orig_target = target) before calling one_hot in the method where self.to_onehot_y is handled, compute the ignore mask from orig_target when it contains integer class labels, and if the supplied target is already one-hot derive the mask from the ignore_index channel (e.g., orig_target[..., ignore_index] or equivalent); apply the same change to the second occurrence of the logic (lines matching the 167-170 block) so both paths correctly detect ignored voxels regardless of whether target was converted or already one-hot.monai/losses/dice.py (1)
169-177:⚠️ Potential issue | 🔴 CriticalBuild the ignore mask from a sanitized label view.
At Line 171 the mask is based on tensor values, so it only works for single-channel sentinel maps. For documented one-hot targets,
ignore_indexnever matches class membership, and withinclude_background=FalsetheC-channel mask no longer broadcasts after Lines 183-185 sliceinputandtarget. Also sanitize ignored labels before Line 177’sone_hot.Also applies to: 179-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/losses/dice.py` around lines 169 - 177, Build the ignore mask from a sanitized label view before converting to one-hot: compute a boolean label_mask = (target != self.ignore_index) (or all-True if ignore_index is None), then if self.to_onehot_y is True sanitize target for one_hot by copying target and setting ignored positions to a valid class (e.g., 0) before calling one_hot; after one_hot, expand/reshape label_mask into the channel dimension to match target/prediction (e.g., mask = label_mask.unsqueeze(1).expand_as(target).float()) so the mask works for both single-channel sentinel maps and multi-channel one-hot targets and still broadcasts correctly when include_background slicing happens (apply same mask generation in the later block covering lines ~179-192).monai/metrics/utils.py (3)
316-326:⚠️ Potential issue | 🟡 Minor
ignore_indexparameter unused inget_edge_surface_distance.Parameter at line 325 is accepted but not forwarded to
get_mask_edgesat line 355 or used elsewhere in the function.🛠️ Forward ignore_index to get_mask_edges
- edge_results = get_mask_edges(y_pred, y, crop=True, spacing=edges_spacing, always_return_as_numpy=False) + edge_results = get_mask_edges(y_pred, y, crop=True, spacing=edges_spacing, always_return_as_numpy=False, ignore_index=ignore_index)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 316 - 326, The get_edge_surface_distance function accepts ignore_index but never forwards it to get_mask_edges or uses it; update get_edge_surface_distance to pass the ignore_index argument through when calling get_mask_edges (the call site around get_mask_edges in get_edge_surface_distance) so masked edge computation respects ignore_index, and ensure any signatures (get_mask_edges invocation) and downstream uses handle a None value appropriately.
265-271:⚠️ Potential issue | 🟡 Minor
maskparameter added but unused.The
maskparameter at line 270 is not used inget_surface_distance. The function computes distances based on edges but doesn't apply the mask. Remove or implement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 265 - 271, get_surface_distance currently accepts a mask parameter but never uses it; either remove the parameter or apply it to limit where surface distances are computed. To fix, update get_surface_distance so that before computing edges (from seg_pred and seg_gt) you validate and cast mask to a boolean array matching seg_pred/seg_gt shape and then apply it to zero-out/ignore voxels outside the mask (e.g., mask the inputs seg_pred and seg_gt or filter edge point sets) so that subsequent edge extraction and distance calculations only consider masked regions; keep the existing logic for distance_metric and spacing unchanged and ensure mask handling is documented in the function signature and tests.
161-169:⚠️ Potential issue | 🟡 Minor
ignore_indexparameter is unused and undocumented.The parameter at line 168 is never referenced in the function body and missing from the docstring's Args section. Either implement the functionality and document it, or remove the parameter to keep the API honest.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 161 - 169, The ignore_index parameter in get_mask_edges is never used and not documented; remove it from the function signature and any related type hints, delete its mention in the docstring Args section, and update any call sites/tests that pass ignore_index to stop supplying it; alternatively if you prefer keeping behavior, implement logic inside get_mask_edges to mask out label==ignore_index from seg_gt/seg_pred before processing and document the parameter in the docstring—choose one approach and make matching changes to callers and tests for consistency.
🧹 Nitpick comments (2)
monai/metrics/hausdorff_distance.py (1)
199-207: Redundant masking when bothignore_indexandmaskare used.At line 105-107,
y_predandyare already masked. Then at lines 200-202,ypandytare masked again withvalid_mask. Themask[b, 0]is also passed toget_edge_surface_distanceat line 215. This triple application may be intentional for robustness but adds overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/hausdorff_distance.py` around lines 199 - 207, The code redundantly reapplies masking: y_pred/y are already masked earlier (mask[b, 0]) yet yp/yt are masked again with valid_mask before computing distances and the same mask is later passed into get_edge_surface_distance; remove the duplicated masking to avoid overhead by eliminating the yp = yp * valid_mask and yt = yt * valid_mask lines (or skip the earlier mask and only apply a single, consistent mask) and ensure the mask passed to get_edge_surface_distance is the same single mask used to prepare yp/yt; update references in the loop that set hd[b, c] and the early all-ignored check to use that single mask (valid_mask or mask[b, 0]) so masking occurs exactly once.monai/metrics/generalized_dice.py (1)
198-204: Per-batch loop for inf replacement is functional but could be vectorized.Works correctly but iterating per batch may be slower for large batches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/generalized_dice.py` around lines 198 - 204, Loop over batches replacing infs in w_full is correct but can be vectorized: compute inf_mask = torch.isinf(w_full), create temp = w_full.masked_fill(inf_mask, 0), compute per-batch max_w = temp.view(w_full.shape[0], -1).max(dim=1).values, build per-batch replacement values = torch.where(max_w > 0, max_w, torch.tensor(1.0, device=w_full.device)), expand/broadcast those replacements to the shape of w_full and assign them into w_full at inf_mask; update the code replacing the per-batch for-loop (variables: w_full, batch_w, infs) with this vectorized sequence so behavior remains identical.
🤖 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/losses/focal_loss.py`:
- Around line 167-171: The current implementation applies the ignore mask to
logits/targets (input/target) which still yields a nonzero BCE/focal term for
masked entries; instead, compute the elementwise focal/BCE loss normally (in the
sigmoid and other paths) and then multiply that per-element loss by mask to zero
out ignored voxels, and when reducing use sum(loss * mask) / mask.sum() (or
clamp minimal denom) so ignored elements don't contribute to numerator or
denominator; update the forward method in FocalLoss (and the other masked blocks
referenced around the sigmoid branch and the regions you noted at 183-185 and
207-218) to apply mask to the computed loss tensor and perform masked reduction
rather than masking input/target or using .mean() over masked values.
In `@monai/losses/tversky.py`:
- Around line 135-136: original_target is preserved but one_hot is called on the
raw target which can contain out-of-range sentinel values (e.g., 255) causing
one_hot to fail; before calling one_hot(target, num_classes=n_pred_ch) replace
ignored/sentinel labels in target with a valid class id (e.g., 0 or another safe
class index) then call one_hot using that sanitized target, and continue to
derive the ignore mask from original_target (not the sanitized target) when
constructing the mask and applying it in the subsequent loss logic (refer to
original_target, target, one_hot, n_pred_ch, and the mask construction used in
the block around lines 138-148).
In `@monai/losses/unified_focal_loss.py`:
- Around line 167-172: The mean-reduction branch incorrectly divides loss.sum()
by spatial_mask.sum(), undercounting channels (loss has shape [B,2,H,W]); update
the normalization to account for both channel elements by either applying the
spatial mask to loss (e.g., expand spatial_mask to match loss with
spatial_mask.unsqueeze(1).expand_as(loss)), summing the masked loss and dividing
by the masked-element count, or divide loss.sum() by (spatial_mask.sum() *
loss.size(1)). Make the change in the block handling self.reduction ==
LossReduction.MEAN.value and self.ignore_index is not None (affecting variables
loss, spatial_mask, back_ce, fore_ce).
- Around line 71-77: The current mask uses (torch.sum(y_true, dim=1) > 0) which
does not respect ignore_index; update the ignore handling to build the mask from
the original label indices or from the explicit ignored-class channel instead of
summing one-hot channels. Concretely: when ignore_index is set, if you have
access to the original label tensor (e.g., labels / y_true_indices) create
valid_mask = (labels != self.ignore_index).unsqueeze(1) and expand it to y_true
shape; otherwise (when y_true is one-hot) build the mask from the ignored class
channel as valid_mask = 1 - y_true[:, self.ignore_index:self.ignore_index+1,
...] and use that to mask y_pred and other computations (replace spatial_mask
usage in this file and in AsymmetricFocalTverskyLoss code paths).
- Around line 157-160: The ignore_index handling currently builds spatial_mask
from torch.sum(y_true, dim=1) which fails when self.to_onehot_y is False; update
the block in AsymmetricFocalLoss (unified_focal_loss.py) to branch on
self.to_onehot_y: when to_onehot_y is False compute spatial_mask = (y_true !=
self.ignore_index).unsqueeze(1).float() (so label tensors drop ignored voxels),
otherwise keep the existing one-hot style mask (e.g., spatial_mask =
(torch.sum(y_true, dim=1, keepdim=True) > 0).float()); then apply that
spatial_mask to cross_entropy as before.
In `@monai/metrics/generalized_dice.py`:
- Around line 157-161: The current ignore_index mask in
monai/metrics/generalized_dice.py compares a one-hot y to an integer and always
yields True; change the masking to detect one-hot class vectors using n_class:
if ignore_index >= n_class set mask = (y.sum(dim=1, keepdim=True) > 0).float(),
else set mask = (1.0 - y[:, ignore_index:ignore_index+1]).float(); then apply
that mask to y_pred and y as before (y_pred = y_pred * mask; y = y * mask). Use
the existing variables y, y_pred, ignore_index and n_class to implement this
branching so one-hot inputs are masked correctly.
In `@monai/metrics/hausdorff_distance.py`:
- Around line 151-152: The docstring for hausdorff_distance is missing
documentation for the mask parameter; update the function's docstring to
describe mask: its type (torch.Tensor | None), expected shape (same spatial
shape as y_pred/y), semantics (binary or boolean tensor where True/non-zero
marks voxels to include in the metric; if None, no spatial masking is applied),
how it interacts with ignore_index (e.g., masked-out voxels are excluded before
applying ignore_index), and any dtype/format expectations (boolean or 0/1
integer). Mention that mask applies to both prediction and ground-truth when
computing distances and that mask and ignore_index can be used together (specify
precedence if relevant).
In `@monai/metrics/meaniou.py`:
- Around line 146-151: compute_iou's ignore_index handling assumes label-encoded
y but must support one-hot y; change the mask logic in compute_iou to detect
one-hot (y.dim()==y_pred.dim() and y.shape[1]==y_pred.shape[1] or otherwise by
checking channel count) and, when one-hot, build a [B,1,...] mask from y[:,
ignore_index:ignore_index+1] (e.g., mask = 1 - y[:,
ignore_index:ignore_index+1]) then expand_as(y_pred) and apply it to y_pred; for
label-encoded y keep the existing mask = (y != ignore_index).float() and replace
y values with 0 via torch.where(y == ignore_index, torch.tensor(0,
device=y.device), y) only in that branch so one-hot y is not altered
incorrectly.
In `@monai/metrics/surface_dice.py`:
- Around line 223-227: The current one-hot mask logic using (y !=
ignore_index).all(dim=1, keepdim=True) is incorrect for one-hot encoded labels;
replace it by constructing a class-based mask from the one-hot channel: build
mask = (1 - y[:, ignore_index:ignore_index+1]).float() (preserving a [B,1,...]
shape) and then multiply y_pred and y by that mask; update the block that checks
ignore_index and applies masking to y_pred and y (referencing y, y_pred, and
ignore_index) so ignored-class voxels are zeroed correctly.
In `@monai/metrics/surface_distance.py`:
- Around line 174-176: The current conditional skips calling ignore_background
when include_background is False and ignore_index == 0, causing background
removal to be bypassed unintentionally; update the logic in surface_distance.py
so that when include_background is False you always call
ignore_background(y_pred=y_pred, y=y) (remove or change the ignore_index check),
or alternatively make the behavior explicit by documenting that ignore_index==0
preserves background; reference the include_background flag, ignore_index
variable, and the ignore_background(y_pred=y_pred, y=y) call when making the
change.
In `@monai/metrics/utils.py`:
- Around line 358-364: The mask-slicing branch currently only runs when
edge_results[2] is a tuple, so in subvoxel mode (when spacing is provided and
edge_results[2] is an area tensor) the mask is never applied; update the code
around edge_results/ mask handling so it handles both cases: if edge_results[2]
is a tuple extract slices and do mask = mask[slices], otherwise skip slicing and
just move/cast the mask to the edges_pred device (mask =
mask.to(edges_pred.device).bool()), then apply edges_pred = edges_pred & mask
and edges_gt = edges_gt & mask. Ensure you reference edge_results, mask,
edges_pred and edges_gt when making the change.
In `@tests/losses/test_ignore_index_losses.py`:
- Around line 22-27: The current TEST_CASES only checks invariance with ignored
indices but not that ignored voxels are truly excluded; update the tests in
tests/losses/test_ignore_index_losses.py to compute each loss twice—(1) the
existing full-volume loss with ignore_index set and (2) a reference loss
computed only on the non-ignored region (mask out ignored voxels from y_true and
y_pred and/or slice to the non-ignored voxels) and assert those values match—so
constant penalties from ignored voxels would fail; extend TEST_CASES to include
both to_onehot_y=True and an already-one-hot label case for each Loss class
(DiceLoss, FocalLoss, TverskyLoss, AsymmetricUnifiedFocalLoss) and ensure the
test covers both softmax/sigmoid variants referenced in the tuple entries.
In `@tests/metrics/test_ignore_index_metrics.py`:
- Around line 48-50: The class-level `@unittest.skipUnless`(has_scipy, ...) is
incorrectly skipping metrics that don't require SciPy; update the test split so
non-SciPy metrics run regardless. Remove or change the decorator on
TestIgnoreIndexMetrics and have it use `@parameterized.expand`(TEST_METRICS) only,
then create a separate test class (e.g., TestIgnoreIndexSurfaceMetrics)
decorated with `@unittest.skipUnless`(has_scipy, ...) that uses
`@parameterized.expand`(SCIPY_METRICS) so only the surface-distance tests are
gated on has_scipy.
- Around line 63-66: The test currently fills all channels with 255 which never
triggers the class-based one-hot path; change the bottom-half sentinel
assignment to a proper one-hot vector for the ignored class so the masking logic
is exercised—specifically, update the tensor y (created in the test) so that for
the ignored class channel (referenced as the channel index used by the
metric/ignored_class constant) you set y[:, ignored_channel, 2:4, :] = 1 and
ensure the other channels in that region remain 0; keep the top-half valid
labels (y[:, 1, 0:2, 0:2] = 1.0) intact so both valid and ignored-class regions
are present and the class-based one-hot branch in the metric (the one handling
one-hot label tensors) is covered.
---
Outside diff comments:
In `@monai/losses/dice.py`:
- Around line 169-177: Build the ignore mask from a sanitized label view before
converting to one-hot: compute a boolean label_mask = (target !=
self.ignore_index) (or all-True if ignore_index is None), then if
self.to_onehot_y is True sanitize target for one_hot by copying target and
setting ignored positions to a valid class (e.g., 0) before calling one_hot;
after one_hot, expand/reshape label_mask into the channel dimension to match
target/prediction (e.g., mask =
label_mask.unsqueeze(1).expand_as(target).float()) so the mask works for both
single-channel sentinel maps and multi-channel one-hot targets and still
broadcasts correctly when include_background slicing happens (apply same mask
generation in the later block covering lines ~179-192).
In `@monai/losses/focal_loss.py`:
- Around line 150-154: The ignore_index mask is being computed from the
post-conversion target (after one_hot), so when users pass one-hot targets the
class IDs are lost and ignored voxels aren’t detected; fix by saving the
original target (e.g., orig_target = target) before calling one_hot in the
method where self.to_onehot_y is handled, compute the ignore mask from
orig_target when it contains integer class labels, and if the supplied target is
already one-hot derive the mask from the ignore_index channel (e.g.,
orig_target[..., ignore_index] or equivalent); apply the same change to the
second occurrence of the logic (lines matching the 167-170 block) so both paths
correctly detect ignored voxels regardless of whether target was converted or
already one-hot.
In `@monai/metrics/utils.py`:
- Around line 316-326: The get_edge_surface_distance function accepts
ignore_index but never forwards it to get_mask_edges or uses it; update
get_edge_surface_distance to pass the ignore_index argument through when calling
get_mask_edges (the call site around get_mask_edges in
get_edge_surface_distance) so masked edge computation respects ignore_index, and
ensure any signatures (get_mask_edges invocation) and downstream uses handle a
None value appropriately.
- Around line 265-271: get_surface_distance currently accepts a mask parameter
but never uses it; either remove the parameter or apply it to limit where
surface distances are computed. To fix, update get_surface_distance so that
before computing edges (from seg_pred and seg_gt) you validate and cast mask to
a boolean array matching seg_pred/seg_gt shape and then apply it to
zero-out/ignore voxels outside the mask (e.g., mask the inputs seg_pred and
seg_gt or filter edge point sets) so that subsequent edge extraction and
distance calculations only consider masked regions; keep the existing logic for
distance_metric and spacing unchanged and ensure mask handling is documented in
the function signature and tests.
- Around line 161-169: The ignore_index parameter in get_mask_edges is never
used and not documented; remove it from the function signature and any related
type hints, delete its mention in the docstring Args section, and update any
call sites/tests that pass ignore_index to stop supplying it; alternatively if
you prefer keeping behavior, implement logic inside get_mask_edges to mask out
label==ignore_index from seg_gt/seg_pred before processing and document the
parameter in the docstring—choose one approach and make matching changes to
callers and tests for consistency.
---
Nitpick comments:
In `@monai/metrics/generalized_dice.py`:
- Around line 198-204: Loop over batches replacing infs in w_full is correct but
can be vectorized: compute inf_mask = torch.isinf(w_full), create temp =
w_full.masked_fill(inf_mask, 0), compute per-batch max_w =
temp.view(w_full.shape[0], -1).max(dim=1).values, build per-batch replacement
values = torch.where(max_w > 0, max_w, torch.tensor(1.0, device=w_full.device)),
expand/broadcast those replacements to the shape of w_full and assign them into
w_full at inf_mask; update the code replacing the per-batch for-loop (variables:
w_full, batch_w, infs) with this vectorized sequence so behavior remains
identical.
In `@monai/metrics/hausdorff_distance.py`:
- Around line 199-207: The code redundantly reapplies masking: y_pred/y are
already masked earlier (mask[b, 0]) yet yp/yt are masked again with valid_mask
before computing distances and the same mask is later passed into
get_edge_surface_distance; remove the duplicated masking to avoid overhead by
eliminating the yp = yp * valid_mask and yt = yt * valid_mask lines (or skip the
earlier mask and only apply a single, consistent mask) and ensure the mask
passed to get_edge_surface_distance is the same single mask used to prepare
yp/yt; update references in the loop that set hd[b, c] and the early all-ignored
check to use that single mask (valid_mask or mask[b, 0]) so masking occurs
exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de4d36bb-f303-47c9-bc69-6339e0ccdc88
📒 Files selected for processing (14)
monai/losses/dice.pymonai/losses/focal_loss.pymonai/losses/tversky.pymonai/losses/unified_focal_loss.pymonai/metrics/confusion_matrix.pymonai/metrics/generalized_dice.pymonai/metrics/hausdorff_distance.pymonai/metrics/meandice.pymonai/metrics/meaniou.pymonai/metrics/surface_dice.pymonai/metrics/surface_distance.pymonai/metrics/utils.pytests/losses/test_ignore_index_losses.pytests/metrics/test_ignore_index_metrics.py
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Rusheel86
force-pushed
the
feat-ignore-index-support
branch
from
d93f058 to
187da14
Compare
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/metrics/meandice.py (1)
256-287:⚠️ Potential issue | 🟡 MinorDiceHelper docstring missing
ignore_indexparameter.Add ignore_index to the Args section.
📝 Suggested addition
num_classes: number of input channels (always including the background). When this is ``None``, ``y_pred.shape[1]`` will be used. This option is useful when both ``y_pred`` and ``y`` are single-channel class indices and the number of classes is not automatically inferred from data. + ignore_index: class index to ignore during calculation. Defaults to ``None``. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/meandice.py` around lines 256 - 287, The DiceHelper docstring is missing the ignore_index parameter description; update the DiceHelper class docstring (the Args section) to include a brief entry for ignore_index explaining it is an optional int specifying a label to ignore during Dice computation (matching the __init__ signature where ignore_index: int | None = None), and ensure the phrasing and type match other Args entries (e.g., include_background, threshold, apply_argmax) so docs and the __init__ parameters remain consistent.
♻️ Duplicate comments (3)
tests/metrics/test_ignore_index_metrics.py (2)
63-70:⚠️ Potential issue | 🟠 MajorTest only exercises sentinel-value masking, not class-based one-hot path.
With
ignore_index=255and 2 channels, the conditionignore_index < y.shape[1]is always false. Add a test case withignore_index=1(a valid class index) where the ignored channel is set to 1 to exercise the class-based masking branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/metrics/test_ignore_index_metrics.py` around lines 63 - 70, The test currently only covers sentinel-style masking (ignore_index=255) by leaving the bottom region as all zeros; add an additional test case that uses ignore_index=1 to exercise the class-based one-hot masking path: create a new y tensor (same shape as current y) and set the ignored region pixels to the one-hot class channel (e.g., set y[:, 1, 2:4, 0:4] = 1.0 for the bottom half) so the branch guarded by the condition ignore_index < y.shape[1] runs and is asserted against; keep the existing sentinel test and add assertions for both behaviors.
48-50:⚠️ Potential issue | 🟠 MajorDon't gate non-SciPy metrics behind SciPy.
DiceMetric,MeanIoU,GeneralizedDiceScore, andConfusionMatrixMetricdon't require SciPy. Split into two test classes: one forTEST_METRICSwithout the skip decorator, another forSCIPY_METRICSwith it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/metrics/test_ignore_index_metrics.py` around lines 48 - 50, The test class TestIgnoreIndexMetrics is incorrectly gating all metrics behind SciPy; split it into two classes: keep one TestIgnoreIndexMetrics (or TestIgnoreIndexMetricsBasic) that uses `@parameterized.expand`(TEST_METRICS) without the `@unittest.skipUnless`(has_scipy, ...) decorator to cover DiceMetric, MeanIoU, GeneralizedDiceScore, ConfusionMatrixMetric, etc., and create a second class (e.g., TestIgnoreIndexMetricsScipy) decorated with `@unittest.skipUnless`(has_scipy, "Scipy required for surface metrics") that uses `@parameterized.expand`(SCIPY_METRICS) to run SciPy-dependent metrics; update class names and the parameterized.expand calls accordingly so non-SciPy tests run even when SciPy is absent.monai/metrics/utils.py (1)
363-369:⚠️ Potential issue | 🟠 MajorMask slicing condition may not execute in subvoxel mode.
When
use_subvoxels=True,edge_results[2]is an area tensor, not a tuple containing slices. The conditionisinstance(edge_results[2], tuple)will be False, skipping mask application.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 363 - 369, The mask-slicing branch in the block using edge_results[2] only handles the case where edge_results[2] is a tuple of slices, so when use_subvoxels=True and edge_results[2] is an area tensor the mask is never applied; update the condition to handle both shapes: if edge_results[2] is a tuple treat it as slices (as now), else if it's a torch.Tensor treat it as an area mask—convert it to the target device and dtype (torch.bool) and then apply it to edges_pred and edges_gt (e.g., mask = mask & area_tensor); adjust the code around edge_results, mask, edges_pred, and edges_gt to ensure both paths produce a boolean mask on the same device before combining.
🧹 Nitpick comments (3)
monai/metrics/utils.py (1)
72-90:ignore_index_maskdocstring incomplete.Missing Args, Returns sections per Google-style docstring guidelines.
📝 Suggested docstring
def ignore_index_mask( y_pred: torch.Tensor, y: torch.Tensor, ignore_index: int | None = None ) -> tuple[torch.Tensor, torch.Tensor]: """ Masks out the specified ignore_index from both predictions and ground truth. - This is a helper for `#8667` to allow 'Ignore Class' functionality in metrics. + + Args: + y_pred: Prediction tensor. + y: Ground truth tensor. + ignore_index: Class index to ignore. If None, inputs returned unchanged. + + Returns: + Tuple of masked (y_pred, y) tensors. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 72 - 90, The docstring for ignore_index_mask is missing Google-style Args and Returns sections; update the docstring of ignore_index_mask to include an Args section documenting y_pred: torch.Tensor (predictions tensor), y: torch.Tensor (ground-truth tensor), and ignore_index: int | None (index to mask out, None means no masking), and a Returns section describing the tuple[torch.Tensor, torch.Tensor] (masked y_pred and masked y); keep the short description and mention that the mask zeros out positions where y == ignore_index and that when ignore_index is None the inputs are returned unchanged.monai/metrics/surface_dice.py (1)
284-298: Defensive handling of areas tuple is reasonable.The code handles edge cases where areas might be a single tensor or varying tuple lengths. Consider adding a comment explaining why this defensive check is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/surface_dice.py` around lines 284 - 298, Add a short inline comment above the defensive block that handles the variable areas in surface_dice.py explaining why the check is necessary: note that the upstream area computation can return either a single tensor (applies to both pred and gt), a tuple/list of two tensors (areas_pred, areas_gt), or occasionally other lengths (handled as empty tensors), and that the guard ensures subsequent indexing by edges_gt/edges_pred is safe; reference the variables areas, areas_pred, and areas_gt so future readers know exactly which values and shapes are being normalized here.tests/metrics/test_ignore_index_metrics.py (1)
51-87: Add docstrings for class and test method.As per coding guidelines, docstrings should describe the test purpose and parameters.
📝 Suggested docstring
class TestIgnoreIndexMetrics(unittest.TestCase): + """Tests that metrics produce identical results when predictions differ only in ignored regions.""" + `@parameterized.expand`(TEST_METRICS + SCIPY_METRICS) def test_metric_ignore_consistency(self, metric_class, kwargs): + """ + Verify ignore_index excludes specified regions from metric computation. + + Args: + metric_class: The metric class to test. + kwargs: Initialization arguments for the metric. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/metrics/test_ignore_index_metrics.py` around lines 51 - 87, Add clear docstrings for the test class (the class that contains test_metric_ignore_consistency) and for the test method test_metric_ignore_consistency: the class docstring should state the purpose of the test suite (testing ignore_index behavior for metrics) and any shared fixtures/parameters, and the method docstring should describe the specific scenario (two predictions differing only in ignored regions, ignore_index=255 sentinel, expected identical aggregated results) plus the method inputs (metric_class, kwargs) and expected outcome; place the docstrings immediately above the class definition and above the test_metric_ignore_consistency function respectively.
🤖 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/generalized_dice.py`:
- Around line 140-141: The docstring for the generalized_dice function has a
parameter name mismatch: it documents sum_over_labels but the actual parameter
is sum_over_classes; update the docstring to use sum_over_classes (or rename the
function parameter to sum_over_labels if you prefer the alternate API) so the
parameter names match; edit the docstring in generalized_dice (and any related
references in the same file) to reflect sum_over_classes for the described
behavior.
In `@monai/metrics/utils.py`:
- Line 330: The parameter ignore_index in get_edge_surface_distance is declared
but never used; remove it from the function signature, update any internal
references (docstring, type hints) and update all callers to stop passing
ignore_index, ensuring function behavior and tests remain unchanged; if the
ignore behavior was intended, instead implement masking using ignore_index
inside get_edge_surface_distance where label arrays are processed (e.g., filter
out voxels/labels equal to ignore_index) rather than leaving the parameter
unused.
- Line 168: The parameter ignore_index is declared but unused in the function in
monai/metrics/utils.py; remove the unused parameter named ignore_index from the
function signature, delete any mention of it in the function docstring/type
hints, and update any internal type annotations or default values that
referenced it; also search for and update any call sites or wrappers that pass
ignore_index so they no longer supply this argument (or adjust them to use the
updated signature) and run tests to ensure no callers remain.
---
Outside diff comments:
In `@monai/metrics/meandice.py`:
- Around line 256-287: The DiceHelper docstring is missing the ignore_index
parameter description; update the DiceHelper class docstring (the Args section)
to include a brief entry for ignore_index explaining it is an optional int
specifying a label to ignore during Dice computation (matching the __init__
signature where ignore_index: int | None = None), and ensure the phrasing and
type match other Args entries (e.g., include_background, threshold,
apply_argmax) so docs and the __init__ parameters remain consistent.
---
Duplicate comments:
In `@monai/metrics/utils.py`:
- Around line 363-369: The mask-slicing branch in the block using
edge_results[2] only handles the case where edge_results[2] is a tuple of
slices, so when use_subvoxels=True and edge_results[2] is an area tensor the
mask is never applied; update the condition to handle both shapes: if
edge_results[2] is a tuple treat it as slices (as now), else if it's a
torch.Tensor treat it as an area mask—convert it to the target device and dtype
(torch.bool) and then apply it to edges_pred and edges_gt (e.g., mask = mask &
area_tensor); adjust the code around edge_results, mask, edges_pred, and
edges_gt to ensure both paths produce a boolean mask on the same device before
combining.
In `@tests/metrics/test_ignore_index_metrics.py`:
- Around line 63-70: The test currently only covers sentinel-style masking
(ignore_index=255) by leaving the bottom region as all zeros; add an additional
test case that uses ignore_index=1 to exercise the class-based one-hot masking
path: create a new y tensor (same shape as current y) and set the ignored region
pixels to the one-hot class channel (e.g., set y[:, 1, 2:4, 0:4] = 1.0 for the
bottom half) so the branch guarded by the condition ignore_index < y.shape[1]
runs and is asserted against; keep the existing sentinel test and add assertions
for both behaviors.
- Around line 48-50: The test class TestIgnoreIndexMetrics is incorrectly gating
all metrics behind SciPy; split it into two classes: keep one
TestIgnoreIndexMetrics (or TestIgnoreIndexMetricsBasic) that uses
`@parameterized.expand`(TEST_METRICS) without the `@unittest.skipUnless`(has_scipy,
...) decorator to cover DiceMetric, MeanIoU, GeneralizedDiceScore,
ConfusionMatrixMetric, etc., and create a second class (e.g.,
TestIgnoreIndexMetricsScipy) decorated with `@unittest.skipUnless`(has_scipy,
"Scipy required for surface metrics") that uses
`@parameterized.expand`(SCIPY_METRICS) to run SciPy-dependent metrics; update
class names and the parameterized.expand calls accordingly so non-SciPy tests
run even when SciPy is absent.
---
Nitpick comments:
In `@monai/metrics/surface_dice.py`:
- Around line 284-298: Add a short inline comment above the defensive block that
handles the variable areas in surface_dice.py explaining why the check is
necessary: note that the upstream area computation can return either a single
tensor (applies to both pred and gt), a tuple/list of two tensors (areas_pred,
areas_gt), or occasionally other lengths (handled as empty tensors), and that
the guard ensures subsequent indexing by edges_gt/edges_pred is safe; reference
the variables areas, areas_pred, and areas_gt so future readers know exactly
which values and shapes are being normalized here.
In `@monai/metrics/utils.py`:
- Around line 72-90: The docstring for ignore_index_mask is missing Google-style
Args and Returns sections; update the docstring of ignore_index_mask to include
an Args section documenting y_pred: torch.Tensor (predictions tensor), y:
torch.Tensor (ground-truth tensor), and ignore_index: int | None (index to mask
out, None means no masking), and a Returns section describing the
tuple[torch.Tensor, torch.Tensor] (masked y_pred and masked y); keep the short
description and mention that the mask zeros out positions where y ==
ignore_index and that when ignore_index is None the inputs are returned
unchanged.
In `@tests/metrics/test_ignore_index_metrics.py`:
- Around line 51-87: Add clear docstrings for the test class (the class that
contains test_metric_ignore_consistency) and for the test method
test_metric_ignore_consistency: the class docstring should state the purpose of
the test suite (testing ignore_index behavior for metrics) and any shared
fixtures/parameters, and the method docstring should describe the specific
scenario (two predictions differing only in ignored regions, ignore_index=255
sentinel, expected identical aggregated results) plus the method inputs
(metric_class, kwargs) and expected outcome; place the docstrings immediately
above the class definition and above the test_metric_ignore_consistency function
respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f028bef2-c8c7-47da-9559-bdce60c79ab7
📒 Files selected for processing (7)
monai/losses/unified_focal_loss.pymonai/metrics/generalized_dice.pymonai/metrics/meandice.pymonai/metrics/meaniou.pymonai/metrics/surface_dice.pymonai/metrics/utils.pytests/metrics/test_ignore_index_metrics.py
✅ Files skipped from review due to trivial changes (1)
- monai/metrics/meaniou.py
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/metrics/generalized_dice.py (1)
73-78:⚠️ Potential issue | 🟡 MinorRemove
ignore_indexfrom_compute_tensor’s Args block.The method does not accept that parameter, so the docstring is wrong.
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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/generalized_dice.py` around lines 73 - 78, The docstring for _compute_tensor incorrectly lists an ignore_index argument that the method does not accept; update the Args section of the _compute_tensor docstring in generalized_dice.py by removing the ignore_index entry and ensuring only actual parameters (e.g., y_pred, y) are documented in Google-style format to match the method signature and coding guidelines.
♻️ Duplicate comments (1)
monai/metrics/utils.py (1)
360-366:⚠️ Potential issue | 🔴 CriticalMask is never applied in any mode.
The condition
len(edge_results) > 2 and isinstance(edge_results[2], tuple)cannot be satisfied:
- Non-subvoxel mode:
get_mask_edgesreturns 2 elements →len(edge_results) > 2is False- Subvoxel mode: Returns 4 elements, but
edge_results[2]isareas_pred(a tensor), never a tupleThe mask parameter is accepted but silently ignored.
🐛 Proposed fix
edge_results = get_mask_edges(y_pred, y, crop=True, spacing=edges_spacing, always_return_as_numpy=False) edges_pred, edges_gt = edge_results[0], edge_results[1] if mask is not None: - if len(edge_results) > 2 and isinstance(edge_results[2], tuple): - slices = edge_results[2] - mask = mask[slices] - mask = torch.as_tensor(mask, device=edges_pred.device, dtype=torch.bool) - edges_pred = edges_pred & mask - edges_gt = edges_gt & mask + mask = torch.as_tensor(mask, device=edges_pred.device, dtype=torch.bool) + # Resize mask if cropping changed spatial dimensions + if mask.shape != edges_pred.shape: + # Crop mask to match edges; fallback to no masking if incompatible + try: + slices = tuple(slice(0, s) for s in edges_pred.shape) + mask = mask[slices] + except (IndexError, RuntimeError): + pass # Keep original mask shape + if mask.shape == edges_pred.shape: + edges_pred = edges_pred & mask + edges_gt = edges_gt & mask🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 360 - 366, The mask parameter is accepted but never applied because the code only applies it when edge_results[2] is a tuple; instead, when mask is not None directly apply the provided mask to edges_pred and edges_gt. Replace the conditional that checks edge_results[2] with logic that (1) converts the incoming mask to a torch.bool tensor on edges_pred.device (torch.as_tensor(mask, device=edges_pred.device, dtype=torch.bool)) and (2) masks edges_pred and edges_gt via bitwise-and (edges_pred = edges_pred & mask; edges_gt = edges_gt & mask). Keep references to edge_results, edges_pred, edges_gt and the mask parameter so the mask is actually used regardless of get_mask_edges return shape.
🧹 Nitpick comments (1)
monai/metrics/utils.py (1)
384-396: Defensive code for unreachable cases.Lines 386-390:
get_mask_edgesreturns either 2 or 4 elements, never 3. Thelen(areas) == 1branch is unreachable.Lines 394-396:
convert_to_tensordoes not returnNonefor the given input. This null check is dead code.Consider removing or documenting why these guards exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 384 - 396, Remove the unreachable defensive branches around areas and the unnecessary None-check on convert_to_tensor: since get_mask_edges never returns a single-area case, delete the len(areas) == 1 branch in the use_subvoxels handling and replace the unexpected-length fallback with an explicit assertion or ValueError (referencing the areas variable and use_subvoxels flag) so failures are visible; similarly, assume convert_to_tensor(...) (called as out = convert_to_tensor(((edges_pred, edges_gt), distances, tuple(areas)), device=y_pred.device)) never returns None and remove the if out is None check, letting any unexpected behavior surface as an exception instead.
🤖 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/generalized_dice.py`:
- Around line 158-177: The ignore_index handling is treating negative sentinel
values as class-channel indices causing empty slices and broadcast errors;
modify the guard to only treat ignore_index as a class channel when it satisfies
0 <= ignore_index < y.shape[1] (use this same condition in the initial mask
branch and in the channels_to_use.remove(...) logic), otherwise treat it as a
sentinel (keep the existing sentinel branch). Update the logic around variables
ignore_index, y, y_pred, mask and channels_to_use (in the generalized_dice /
generalized_dice implementation) accordingly and add a regression test that
passes a negative ignore_index (e.g., -1 or -100) to verify no channel slicing
occurs and broadcasting remains valid.
- Around line 184-211: The current fallback weight logic computes w_full from
y_o_full (all channels) so the inf->max_finite substitution can be influenced by
excluded channels; change it to compute and sanitize weights only on the scored
channels: derive w from y_o (already sliced by channels_to_use), convert to
float, compute reciprocal/square reciprocal according to Weight in the same way
you did for w_full, then perform the per-batch inf detection/replacement on that
per-score-channel tensor (variables: y_o, channels_to_use, w_full, w) so w
contains the sanitized weights for scored channels only rather than selecting
channels after sanitizing.
In `@monai/metrics/utils.py`:
- Around line 72-91: The docstring for ignore_index_mask is incomplete and the
function only handles label-encoded tensors; update the Google-style docstring
to include Args (y_pred: torch.Tensor, y: torch.Tensor, ignore_index: int | None
— describe shapes, dtype and that mask is spatial with 1 for kept voxels and 0
for ignored) and Returns (tuple[torch.Tensor, torch.Tensor] — masked y_pred and
y with same shapes), and mention no exceptions raised; also either extend the
function to support one-hot/channel-first inputs (detect if y has channel
dimension like in compute_generalized_dice and apply channel-wise masking by
creating mask from y.argmax or from a channel containing ignore_index) or
explicitly document that only label-encoded targets are supported and reference
compute_generalized_dice for the multi-channel approach; implement the chosen
path in ignore_index_mask so callers get correctly masked tensors.
- Line 287: Remove the orphaned docstring entry that documents a non-existent
"mask" parameter in monai/metrics/utils.py: locate the function docstring that
contains the line "mask: optional boolean mask. Pixels where mask is False will
be ignored in the distance computation." and delete that line (or the
corresponding docstring bullet) so the docstring matches the actual function
signature; do not add a new parameter unless you intentionally modify the
function signature to accept and handle a mask.
- Around line 310-316: The current branch in the function using variables
seg_pred and dis performs an unnecessary None check and can return mismatched
types; update the torch.Tensor branch (where seg_pred is a torch.Tensor) to use
the boolean mask seg_pred.bool() and return a torch.empty(0, dtype=dis.dtype,
device=dis.device) when no elements match (rather than np.empty), and update the
NumPy branch to use seg_pred.astype(bool) and return a numpy.empty((0,),
dtype=dis.dtype) when empty; remove the redundant out is not None checks so both
branches always return the same container type as dis (torch.Tensor or
numpy.ndarray) and preserve dtype/device.
---
Outside diff comments:
In `@monai/metrics/generalized_dice.py`:
- Around line 73-78: The docstring for _compute_tensor incorrectly lists an
ignore_index argument that the method does not accept; update the Args section
of the _compute_tensor docstring in generalized_dice.py by removing the
ignore_index entry and ensuring only actual parameters (e.g., y_pred, y) are
documented in Google-style format to match the method signature and coding
guidelines.
---
Duplicate comments:
In `@monai/metrics/utils.py`:
- Around line 360-366: The mask parameter is accepted but never applied because
the code only applies it when edge_results[2] is a tuple; instead, when mask is
not None directly apply the provided mask to edges_pred and edges_gt. Replace
the conditional that checks edge_results[2] with logic that (1) converts the
incoming mask to a torch.bool tensor on edges_pred.device (torch.as_tensor(mask,
device=edges_pred.device, dtype=torch.bool)) and (2) masks edges_pred and
edges_gt via bitwise-and (edges_pred = edges_pred & mask; edges_gt = edges_gt &
mask). Keep references to edge_results, edges_pred, edges_gt and the mask
parameter so the mask is actually used regardless of get_mask_edges return
shape.
---
Nitpick comments:
In `@monai/metrics/utils.py`:
- Around line 384-396: Remove the unreachable defensive branches around areas
and the unnecessary None-check on convert_to_tensor: since get_mask_edges never
returns a single-area case, delete the len(areas) == 1 branch in the
use_subvoxels handling and replace the unexpected-length fallback with an
explicit assertion or ValueError (referencing the areas variable and
use_subvoxels flag) so failures are visible; similarly, assume
convert_to_tensor(...) (called as out = convert_to_tensor(((edges_pred,
edges_gt), distances, tuple(areas)), device=y_pred.device)) never returns None
and remove the if out is None check, letting any unexpected behavior surface as
an exception instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcfb7f36-019d-44f4-a4d6-f43e54f47616
📒 Files selected for processing (2)
monai/metrics/generalized_dice.pymonai/metrics/utils.py
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/metrics/generalized_dice.py (1)
69-92:⚠️ Potential issue | 🟡 MinorDocstring lists non-existent parameter.
Line 77 documents
ignore_indexas an argument to_compute_tensor, but the function signature only acceptsy_predandy. The value comes fromself.ignore_index.📝 Fix
Args: y_pred (torch.Tensor): Binarized segmentation model output. It must be in one-hot format and in the NCHW[D] format, where N is the batch dimension, C is the channel dimension, and the remaining are the spatial dimensions. y (torch.Tensor): Binarized ground-truth. It must be in one-hot format and have the same shape as `y_pred`. - ignore_index: class index to ignore from the metric computation. Returns: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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/generalized_dice.py` around lines 69 - 92, The docstring for _compute_tensor incorrectly lists an `ignore_index` parameter even though the method signature only has y_pred and y and uses self.ignore_index; update the docstring to remove the `ignore_index` parameter entry (or change it to document that ignore_index is taken from self.ignore_index) so the arguments section matches the function signature and mention that ignore_index is read from self.ignore_index; ensure the Args section only documents y_pred, y and any class-sourced options like self.ignore_index under a brief note.
♻️ Duplicate comments (1)
monai/metrics/utils.py (1)
368-374:⚠️ Potential issue | 🟠 MajorMask not applied in subvoxel mode.
edge_results[2]is an area tensor whenspacingis provided, not a tuple of slices. Theisinstance(edge_results[2], tuple)check fails, so lines 370-374 never execute in subvoxel mode.🔧 Suggested fix
if mask is not None: - if len(edge_results) > 2 and isinstance(edge_results[2], tuple): - slices = edge_results[2] - mask = mask[slices] - mask = torch.as_tensor(mask, device=edges_pred.device, dtype=torch.bool) - edges_pred = edges_pred & mask - edges_gt = edges_gt & mask + mask = torch.as_tensor(mask, device=edges_pred.device, dtype=torch.bool) + # Crop mask to match edges shape if needed + if mask.shape != edges_pred.shape: + # Resize/crop mask to match edge dimensions + slices = tuple(slice(0, min(m, e)) for m, e in zip(mask.shape, edges_pred.shape)) + mask = mask[slices] + edges_pred = edges_pred & mask + edges_gt = edges_gt & mask🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 368 - 374, The mask application branch currently only runs when edge_results[2] is a tuple of slices, so in subvoxel mode (where edge_results[2] is an area tensor) the mask never gets applied; update the code in the block handling mask, edge_results and edges_pred/edges_gt so it handles both cases: if edge_results[2] is a tuple treat it as slices (slices = edge_results[2] and mask = mask[slices]), otherwise treat edge_results[2] as an area tensor (derive a boolean mask from that tensor, e.g. area_tensor > 0 or nonzero as appropriate), then cast the mask to torch.bool on edges_pred.device and apply edges_pred = edges_pred & mask and edges_gt = edges_gt & mask; refer to variables edge_results, mask, slices, edges_pred and edges_gt to locate and modify the logic.
🧹 Nitpick comments (1)
monai/metrics/utils.py (1)
385-398: Minor: Unnecessary None guard.
get_surface_distancereturns a tensor (possibly empty or inf-filled), neverNone. Thed if d is not None else ...check on line 385 is defensive but unreachable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 385 - 398, The None check in building distances_list is unnecessary because get_surface_distance never returns None; update the construction around distances_list/distances_raw to remove the d is not None guard and directly use the tensors (e.g., replace the comprehension creating distances_list with one that uses d from distances_raw directly, keeping the fallback of edges_pred.new_empty((0,)) only if you still need a safe empty tensor), ensuring downstream variables distances and distances_list remain the same shape/typing used by the rest of the function (references: distances_list, distances_raw, distances, edges_pred, get_surface_distance).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@monai/metrics/generalized_dice.py`:
- Around line 69-92: The docstring for _compute_tensor incorrectly lists an
`ignore_index` parameter even though the method signature only has y_pred and y
and uses self.ignore_index; update the docstring to remove the `ignore_index`
parameter entry (or change it to document that ignore_index is taken from
self.ignore_index) so the arguments section matches the function signature and
mention that ignore_index is read from self.ignore_index; ensure the Args
section only documents y_pred, y and any class-sourced options like
self.ignore_index under a brief note.
---
Duplicate comments:
In `@monai/metrics/utils.py`:
- Around line 368-374: The mask application branch currently only runs when
edge_results[2] is a tuple of slices, so in subvoxel mode (where edge_results[2]
is an area tensor) the mask never gets applied; update the code in the block
handling mask, edge_results and edges_pred/edges_gt so it handles both cases: if
edge_results[2] is a tuple treat it as slices (slices = edge_results[2] and mask
= mask[slices]), otherwise treat edge_results[2] as an area tensor (derive a
boolean mask from that tensor, e.g. area_tensor > 0 or nonzero as appropriate),
then cast the mask to torch.bool on edges_pred.device and apply edges_pred =
edges_pred & mask and edges_gt = edges_gt & mask; refer to variables
edge_results, mask, slices, edges_pred and edges_gt to locate and modify the
logic.
---
Nitpick comments:
In `@monai/metrics/utils.py`:
- Around line 385-398: The None check in building distances_list is unnecessary
because get_surface_distance never returns None; update the construction around
distances_list/distances_raw to remove the d is not None guard and directly use
the tensors (e.g., replace the comprehension creating distances_list with one
that uses d from distances_raw directly, keeping the fallback of
edges_pred.new_empty((0,)) only if you still need a safe empty tensor), ensuring
downstream variables distances and distances_list remain the same shape/typing
used by the rest of the function (references: distances_list, distances_raw,
distances, edges_pred, get_surface_distance).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce651529-39eb-4da6-aba7-5c7fa61a68de
📒 Files selected for processing (2)
monai/metrics/generalized_dice.pymonai/metrics/utils.py
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/metrics/utils.py (1)
344-359:⚠️ Potential issue | 🟡 MinorDocstring is missing the new
maskargument.Please document
maskinArgsand describe how it affects edge/distance computation.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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 344 - 359, Add a new Args entry documenting the mask parameter: state its name and type (optional array-like/bool mask with same spatial shape as y_pred/y, default None), state that when provided voxels where mask is False are ignored (excluded) from edge detection and distance calculations so edges and distances are computed only inside the masked region, and clarify how this affects outputs (edges_pred/edges_gt and distances correspond only to unmasked voxels); update the Args section near y_pred/y and the Returns description for (edges_pred, edges_gt) to mention masked outputs. Ensure the description references the mask behavior consistently with get_surface_distance and the existing parameters (use_subvoxels, spacing, symmetric, class_index).
♻️ Duplicate comments (1)
monai/metrics/utils.py (1)
335-396:⚠️ Potential issue | 🟠 Major
maskis never applied inget_edge_surface_distance.
maskis accepted but unused, so ignored regions still affect surface distances. This can silently break ignore-index behavior in surface/hausdorff metrics.Suggested fix
def get_edge_surface_distance( @@ - edge_results = get_mask_edges(y_pred, y, crop=True, spacing=edges_spacing, always_return_as_numpy=False) + # keep spatial alignment when an external mask is provided + edge_results = get_mask_edges( + y_pred, y, crop=(mask is None), spacing=edges_spacing, always_return_as_numpy=False + ) edges_pred, edges_gt = edge_results[0], edge_results[1] + + if mask is not None: + mask = mask.to(edges_pred.device).bool() + edges_pred = edges_pred & mask + edges_gt = edges_gt & mask @@ - out = convert_to_tensor(((edges_pred, edges_gt), distances, tuple(areas)), device=y_pred.device) # type: ignore[no-any-return] + out = convert_to_tensor(((edges_pred, edges_gt), distances, tuple(areas)), device=y_pred.device) # type: ignore[no-any-return]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 335 - 396, The function get_edge_surface_distance accepts a mask parameter but never applies it; update get_edge_surface_distance to respect mask by applying it to the edge maps before computing distances: after obtaining edges_pred and edges_gt from get_mask_edges, if mask is not None (and matches the cropped shape returned by get_mask_edges) set edge voxels outside the mask to zero (e.g., edges_pred[~mask]=0 and edges_gt[~mask]=0) or alternatively pass the mask into get_mask_edges if that helper supports it; ensure the same device/dtype alignment and handle None mask as currently done so subsequent calls to get_surface_distance use masked edges only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@monai/metrics/utils.py`:
- Around line 344-359: Add a new Args entry documenting the mask parameter:
state its name and type (optional array-like/bool mask with same spatial shape
as y_pred/y, default None), state that when provided voxels where mask is False
are ignored (excluded) from edge detection and distance calculations so edges
and distances are computed only inside the masked region, and clarify how this
affects outputs (edges_pred/edges_gt and distances correspond only to unmasked
voxels); update the Args section near y_pred/y and the Returns description for
(edges_pred, edges_gt) to mention masked outputs. Ensure the description
references the mask behavior consistently with get_surface_distance and the
existing parameters (use_subvoxels, spacing, symmetric, class_index).
---
Duplicate comments:
In `@monai/metrics/utils.py`:
- Around line 335-396: The function get_edge_surface_distance accepts a mask
parameter but never applies it; update get_edge_surface_distance to respect mask
by applying it to the edge maps before computing distances: after obtaining
edges_pred and edges_gt from get_mask_edges, if mask is not None (and matches
the cropped shape returned by get_mask_edges) set edge voxels outside the mask
to zero (e.g., edges_pred[~mask]=0 and edges_gt[~mask]=0) or alternatively pass
the mask into get_mask_edges if that helper supports it; ensure the same
device/dtype alignment and handle None mask as currently done so subsequent
calls to get_surface_distance use masked edges only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e861711-6956-4e26-9b9d-3ef53d9c9c3c
📒 Files selected for processing (2)
monai/metrics/generalized_dice.pymonai/metrics/utils.py
Signed-off-by: Rusheel Sharma <rusheelhere@gmail.com>
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 #8734
Description
This PR introduces comprehensive native support for
ignore_indexin core MONAI losses and metrics, as requested in issue #8734. Theignore_indexparameter allows specific label values (e.g., padding, unlabeled regions, or boundary artifacts) to be excluded from loss and metric calculations which is a critical feature for medical imaging workflows.Implementation Summary
Losses with
ignore_indexsupport:DiceLoss- masks ignored voxels before dice computationFocalLoss- applies masking after one-hot conversionTverskyLoss- consistent masking approach with DiceLossUnifiedFocalLoss- handles ignore_index in binary target conversionMetrics with
ignore_indexsupport:MeanDice&GeneralizedDiceScore- spatial masking before computationMeanIoU- excludes ignored classes from IoU calculationConfusionMatrixMetric- filters ignored indices from confusion matrixHausdorffDistanceMetric- masks boundary computationSurfaceDiceMetric&SurfaceDistanceMetric- handles ignore_index in surface calculationsKey Fixes:
ignore_indextest_ignore_index_losses.pyandtest_ignore_index_metrics.pyTypes of changes
test_ignore_index_losses.py,test_ignore_index_metrics.py)./runtests.sh -f -u --net --coverage./runtests.sh --quick --unittests --disttestsmake htmlcommand in thedocs/folderRelated Issues
Could merge with or addresses similar requirements as #8667 (Ignore Class)