Adding per_component functionality to Surface Distance metric#8855
Open
VijayVignesh1 wants to merge 3 commits into
Open
Adding per_component functionality to Surface Distance metric#8855VijayVignesh1 wants to merge 3 commits into
VijayVignesh1 wants to merge 3 commits into
Conversation
Signed-off-by: Vijay Vignesh Prasad Rao <vijayvigneshp02@gmail.com>
Signed-off-by: Vijay Vignesh Prasad Rao <vijayvigneshp02@gmail.com>
Contributor
📝 WalkthroughWalkthroughThe pull request adds a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
monai/metrics/surface_distance.py (3)
165-192: ⚡ Quick win
compute_average_surface_distancedocstring missingRaises:andReturns:.
per_componentis described, but neither the newValueErrorpaths (shape mismatch, propagated from_compute_tensor) nor the return tensor are documented. AddReturns:andRaises:sections.As per coding guidelines: "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/metrics/surface_distance.py` around lines 165 - 192, Update the docstring of compute_average_surface_distance to include a Returns: section describing the returned tensor (e.g., tensor of shape [B, C] or [B] depending on include_background/per_component and symmetric flags, datatype torch.float32, containing average (or symmetric) surface distances per batch/component) and a Raises: section documenting exceptions propagated from _compute_tensor (e.g., ValueError for shape or channel mismatches, and any other specific errors raised by spacing/metric validation). Mention that per_component affects output granularity and that a ValueError is raised on input shape mismatch (propagated from _compute_tensor). Reference compute_average_surface_distance and _compute_tensor so the maintainer can locate where to add the Returns: and Raises: entries.
43-55: ⚡ Quick winDocstring "Note" block won't render as Sphinx admonition.
Bullets and reST
.. note::need blank lines and indentation. As-is, this renders as a wall of text. AlsoNote:should follow Google-style or use.. note::consistently.📝 Suggested formatting
- The ``per_component=True`` approach computes the Surface Distance on a per-connected component basis in the ground - truth segmentation. This ensures that each component contributes equally to the final metric, regardless of its size. - Traditional Surface Distance can be dominated by large structures, but the per-component method gives a more - balanced evaluation, particularly for small or fragmented objects. This provides a granular assessment of segmentation - quality, which is especially important in cases with multiple disconnected foreground components. - Note: - - The input prediction (`y_pred`) and ground truth (`y`) must both have 2 channels (foreground/background), - with binary segmentation (0 for background, 1 for foreground). That is, this assumes the shape of both prediction - and ground truth is B2HW[D]. - - This method cannot be used with multiclass segmentation. - For more information, refer to the original paper: https://arxiv.org/abs/2410.18684 + The ``per_component=True`` approach computes the Surface Distance on a per-connected component basis in the + ground truth segmentation. This ensures that each component contributes equally to the final metric, + regardless of its size. Traditional Surface Distance can be dominated by large structures, but the + per-component method gives a more balanced evaluation, particularly for small or fragmented objects. + For more information, refer to the original paper: https://arxiv.org/abs/2410.18684 + + .. note:: + - ``y_pred`` and ``y`` must both have 2 channels (background/foreground) with binary values + (shape ``B2HW[D]``). + - Multiclass segmentation is not supported with ``per_component=True``.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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/metrics/surface_distance.py` around lines 43 - 55, The docstring note block in monai/metrics/surface_distance.py is not using proper Sphinx/reST formatting so it renders as plain text; update the docstring to either (a) use a Google-style "Note:" section with a blank line after the section header and indent the following bullet list lines, or (b) replace it with a reST admonition using ".. note::" followed by a blank line and indented bullets, and ensure the bullet list items describing input shape, multiclass restriction, and the paper reference are each on their own indented lines; apply this change in the module/function docstring where the per_component=True description appears so Sphinx will render the note and bullets correctly.
226-256: ⚡ Quick winRepeated 2D/3D slicing ternaries — fold into a single
tuple(slice(...))expression.The same
if ndim == 5 else ...pattern is duplicated three times. A small slice tuple removes the duplication and avoids future drift.♻️ Suggested refactor
- crop_pred = ( - y_pred[b, c][ - min_corner_idx[0] : max_corner_idx[0] + 1, - min_corner_idx[1] : max_corner_idx[1] + 1, - min_corner_idx[2] : max_corner_idx[2] + 1, - ] - if y_pred.ndim == 5 - else y_pred[b, c][ - min_corner_idx[0] : max_corner_idx[0] + 1, min_corner_idx[1] : max_corner_idx[1] + 1 - ] - ) - - crop_label = ( - y[b, c][ - min_corner_idx[0] : max_corner_idx[0] + 1, - min_corner_idx[1] : max_corner_idx[1] + 1, - min_corner_idx[2] : max_corner_idx[2] + 1, - ] - if y.ndim == 5 - else y[b, c][min_corner_idx[0] : max_corner_idx[0] + 1, min_corner_idx[1] : max_corner_idx[1] + 1] - ) - - cc_crop_mask = ( - cc_mask[ - min_corner_idx[0] : max_corner_idx[0] + 1, - min_corner_idx[1] : max_corner_idx[1] + 1, - min_corner_idx[2] : max_corner_idx[2] + 1, - ] - if y_pred.ndim == 5 - else cc_mask[min_corner_idx[0] : max_corner_idx[0] + 1, min_corner_idx[1] : max_corner_idx[1] + 1] - ) + bbox = tuple( + slice(int(lo), int(hi) + 1) for lo, hi in zip(min_corner_idx, max_corner_idx) + ) + crop_pred = y_pred[b, c][bbox] + crop_label = y[b, c][bbox] + cc_crop_mask = cc_mask[bbox]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/metrics/surface_distance.py` around lines 226 - 256, The three duplicated ternary slice expressions (for crop_pred, crop_label, cc_crop_mask) should be replaced by computing a single slice tuple based on the array dimensionality and reusing it for all three; e.g. build slices = (slice(min_corner_idx[0], max_corner_idx[0]+1), slice(min_corner_idx[1], max_corner_idx[1]+1), slice(min_corner_idx[2], max_corner_idx[2]+1)) if y_pred.ndim == 5 else (slice(min_corner_idx[0], max_corner_idx[0]+1), slice(min_corner_idx[1], max_corner_idx[1]+1)) and then use that tuple to index y_pred[b, c], y[b, c], and cc_mask to assign crop_pred, crop_label, and cc_crop_mask respectively.tests/metrics/test_surface_distance.py (2)
234-236: ⚡ Quick win
test_channel_dimensionscovers only one failure mode.Single 4D/3-channel case. Add a few rows: mismatched ranks (4D vs 5D), correct ranks but channels ≠ 2, and matching ranks/channels but mismatched spatial shapes — these are exactly the branches in the new validation block.
🧪 Suggested expansion
- def test_channel_dimensions(self): - with self.assertRaises(ValueError): - SurfaceDistanceMetric(per_component=True)(torch.ones([3, 3, 144, 144]), torch.ones([3, 3, 144, 144])) + `@parameterized.expand`( + [ + ((3, 3, 144, 144), (3, 3, 144, 144)), # too many channels + ((1, 2, 16, 16), (1, 2, 16, 16, 16)), # mismatched rank + ((1, 2, 16, 16), (1, 2, 32, 32)), # mismatched spatial shape + ] + ) + def test_channel_dimensions(self, pred_shape, gt_shape): + with self.assertRaises(ValueError): + SurfaceDistanceMetric(per_component=True)(torch.ones(pred_shape), torch.ones(gt_shape))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/metrics/test_surface_distance.py` around lines 234 - 236, Expand the test_channel_dimensions test to assert ValueError for the other validation branches in SurfaceDistanceMetric: add cases for mismatched tensor ranks (e.g., 4D vs 5D), tensors with correct rank but invalid channel count (channels != 2), and tensors with matching rank and channel count but mismatched spatial shapes; keep using SurfaceDistanceMetric(per_component=True) and self.assertRaises(ValueError) for each new case so all new validation branches are exercised.
224-232: ⚡ Quick winAdd a symmetric/asymmetric sweep and a spacing case to
test_cc_metrics.The other tests parameterize over
symmetricandspacing;per_componentdeserves the same coverage, especially sinceget_edge_surface_distanceis invoked per component with these settings. Also assertresult.deviceliketest_valuedoes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/metrics/test_surface_distance.py` around lines 224 - 232, Update test_cc_metrics to cover symmetric/asymmetric and spacing variations: parametrize the test over symmetric (True/False) and spacing values (e.g., default and non-unit) similar to other tests, instantiate SurfaceDistanceMetric(per_component=True, symmetric=<param>, spacing=<param>), call sd_metric(seg_1, seg_2) and aggregate(reduction="none"), and add an assertion that result.device matches the expected device (as done in test_value). Ensure this exercises get_edge_surface_distance per component by reusing TEST_CASES_CC_METRICS while sweeping the symmetric and spacing parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/metrics/surface_distance.py`:
- Around line 210-215: The ternary inside the pred_empty && label_empty block is
dead code; update the branch in the per_component loop to explicitly set asd[b,
c] = 0.0 when both pred_empty and label_empty, and set asd[b, c] = float("nan")
and continue when exactly one of pred_empty or label_empty is true to avoid
passing zero masks into compute_voronoi_regions_fast; modify the block that
checks pred_empty/label_empty (using variables y_pred, y, and asd) so the
both-empty and one-empty cases are handled explicitly and then continue to the
next iteration.
- Around line 113-123: Collapse the redundant nested validation under the
per_component branch into one concise condition that checks y_pred.ndim and
y.ndim are in (4,5), both have channel dim == 2, and y_pred.shape == y.shape,
and if that fails raise the existing ValueError (include the same descriptive
message referencing tuple(y_pred.shape) and tuple(y.shape)); then remove the
inner re-derived predicates (same_rank, binary_channels, same_shape). Also
update the _compute_tensor docstring to include a "Raises: ValueError" entry
documenting the per_component shape/channel requirements and the error case.
In `@tests/metrics/test_surface_distance.py`:
- Around line 144-181: The test uses inverted variable names y / y_hat (in
TEST_CASES_CC_METRICS) contrary to MONAI convention and the metric signature;
rename y_hat -> y_pred and y -> y_true (or y) throughout the test block and
ensure the pairs are appended in the metric's expected order (pass y_pred first,
then y_true) when calling sd_metric / when adding entries to
TEST_CASES_CC_METRICS so the first tensor is the prediction and the second is
the ground truth; update all occurrences in this snippet (the five
TEST_CASES_CC_METRICS.append blocks) to mirror the metric argument order.
---
Nitpick comments:
In `@monai/metrics/surface_distance.py`:
- Around line 165-192: Update the docstring of compute_average_surface_distance
to include a Returns: section describing the returned tensor (e.g., tensor of
shape [B, C] or [B] depending on include_background/per_component and symmetric
flags, datatype torch.float32, containing average (or symmetric) surface
distances per batch/component) and a Raises: section documenting exceptions
propagated from _compute_tensor (e.g., ValueError for shape or channel
mismatches, and any other specific errors raised by spacing/metric validation).
Mention that per_component affects output granularity and that a ValueError is
raised on input shape mismatch (propagated from _compute_tensor). Reference
compute_average_surface_distance and _compute_tensor so the maintainer can
locate where to add the Returns: and Raises: entries.
- Around line 43-55: The docstring note block in
monai/metrics/surface_distance.py is not using proper Sphinx/reST formatting so
it renders as plain text; update the docstring to either (a) use a Google-style
"Note:" section with a blank line after the section header and indent the
following bullet list lines, or (b) replace it with a reST admonition using "..
note::" followed by a blank line and indented bullets, and ensure the bullet
list items describing input shape, multiclass restriction, and the paper
reference are each on their own indented lines; apply this change in the
module/function docstring where the per_component=True description appears so
Sphinx will render the note and bullets correctly.
- Around line 226-256: The three duplicated ternary slice expressions (for
crop_pred, crop_label, cc_crop_mask) should be replaced by computing a single
slice tuple based on the array dimensionality and reusing it for all three; e.g.
build slices = (slice(min_corner_idx[0], max_corner_idx[0]+1),
slice(min_corner_idx[1], max_corner_idx[1]+1), slice(min_corner_idx[2],
max_corner_idx[2]+1)) if y_pred.ndim == 5 else (slice(min_corner_idx[0],
max_corner_idx[0]+1), slice(min_corner_idx[1], max_corner_idx[1]+1)) and then
use that tuple to index y_pred[b, c], y[b, c], and cc_mask to assign crop_pred,
crop_label, and cc_crop_mask respectively.
In `@tests/metrics/test_surface_distance.py`:
- Around line 234-236: Expand the test_channel_dimensions test to assert
ValueError for the other validation branches in SurfaceDistanceMetric: add cases
for mismatched tensor ranks (e.g., 4D vs 5D), tensors with correct rank but
invalid channel count (channels != 2), and tensors with matching rank and
channel count but mismatched spatial shapes; keep using
SurfaceDistanceMetric(per_component=True) and self.assertRaises(ValueError) for
each new case so all new validation branches are exercised.
- Around line 224-232: Update test_cc_metrics to cover symmetric/asymmetric and
spacing variations: parametrize the test over symmetric (True/False) and spacing
values (e.g., default and non-unit) similar to other tests, instantiate
SurfaceDistanceMetric(per_component=True, symmetric=<param>, spacing=<param>),
call sd_metric(seg_1, seg_2) and aggregate(reduction="none"), and add an
assertion that result.device matches the expected device (as done in
test_value). Ensure this exercises get_edge_surface_distance per component by
reusing TEST_CASES_CC_METRICS while sweeping the symmetric and spacing
parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bbf43fa-26d6-4723-9a04-39c26f9913a1
📒 Files selected for processing (2)
monai/metrics/surface_distance.pytests/metrics/test_surface_distance.py
Signed-off-by: Vijay Vignesh Prasad Rao <vijayvigneshp02@gmail.com>
VijayVignesh1
changed the title
VijayVignesh1
changed the title
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 #8733
Description
This PR adds support for connected component-based Surface Distance metric calculation to the existing SurfaceDistanceMetric and its helper function.
Changes
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.