Add BoundaryLoss#8916
Conversation
📝 WalkthroughWalkthroughThis PR introduces Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/losses/test_boundary_loss.py`:
- Around line 151-154: Add a Google-style docstring to the helper function
_describe_test_case that documents the parameters and return value: describe the
arguments (test_func, test_number, params) and that params.args unpacks to
(input_param, input_data, _), explain the expected structure of input_data (a
dict with 'input' Tensor including shape and device), and state that the
function returns a formatted string describing the params, shape, and device;
place the docstring immediately under the def line in Google style with Args:
and Returns: sections.
🪄 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: 8cc5f9a0-8eb4-430a-bb9e-4ca7e4e8938d
📒 Files selected for processing (4)
docs/source/losses.rstmonai/losses/__init__.pymonai/losses/boundary_loss.pytests/losses/test_boundary_loss.py
| def _describe_test_case(test_func, test_number, params): | ||
| input_param, input_data, _ = params.args | ||
| return f"params:{input_param}, shape:{input_data['input'].shape}, device:{input_data['input'].device}" | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a Google-style docstring to _describe_test_case.
This helper definition is missing a docstring describing parameters and return value.
Proposed fix
def _describe_test_case(test_func, test_number, params):
+ """Build a readable label for a parameterized test case.
+
+ Args:
+ test_func: The parameterized test function.
+ test_number: The generated case index.
+ params: Parameter bundle containing test args.
+
+ Returns:
+ A string describing params, tensor shape, and device.
+ """
input_param, input_data, _ = params.args
return f"params:{input_param}, shape:{input_data['input'].shape}, device:{input_data['input'].device}"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."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _describe_test_case(test_func, test_number, params): | |
| input_param, input_data, _ = params.args | |
| return f"params:{input_param}, shape:{input_data['input'].shape}, device:{input_data['input'].device}" | |
| def _describe_test_case(test_func, test_number, params): | |
| """Build a readable label for a parameterized test case. | |
| Args: | |
| test_func: The parameterized test function. | |
| test_number: The generated case index. | |
| params: Parameter bundle containing test args. | |
| Returns: | |
| A string describing params, tensor shape, and device. | |
| """ | |
| input_param, input_data, _ = params.args | |
| return f"params:{input_param}, shape:{input_data['input'].shape}, device:{input_data['input'].device}" |
🤖 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/losses/test_boundary_loss.py` around lines 151 - 154, Add a
Google-style docstring to the helper function _describe_test_case that documents
the parameters and return value: describe the arguments (test_func, test_number,
params) and that params.args unpacks to (input_param, input_data, _), explain
the expected structure of input_data (a dict with 'input' Tensor including shape
and device), and state that the function returns a formatted string describing
the params, shape, and device; place the docstring immediately under the def
line in Google style with Args: and Returns: sections.
Source: Coding guidelines
Signed-off-by: Colin Son <txmed82@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
monai/losses/boundary_loss.py (2)
109-124: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocstring missing
Raisessection.Method raises
ValueErrorat line 124 but this isn't documented.Proposed fix
Args: target: target tensor of shape BNHW[D], with values in {0, 1} (one-hot encoded). Returns: Signed distance map of the same shape as target. + + Raises: + ValueError: If target is not 4D (BNHW) or 5D (BNHWD). """ if target.dim() not in (4, 5): raise ValueError("Only 2D (BNHW) and 3D (BNHWD) supported")🤖 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/losses/boundary_loss.py` around lines 109 - 124, The compute_distance_map method raises a ValueError when the target tensor dimensions are not 4 or 5, but this exception is not documented in the docstring. Add a Raises section to the docstring of compute_distance_map that documents the ValueError exception along with a description of when it is raised (specifically when target.dim() is not in (4, 5)).Source: Coding guidelines
147-168: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocstring missing
Returnssection.Per coding guidelines, document the return value.
Proposed fix
Raises: ValueError: If the input is not 2D (BNHW) or 3D (BNHWD). AssertionError: When input and target (after one hot transform if set) have different shapes. ValueError: When ``self.reduction`` is not one of ["mean", "sum", "none"]. + + Returns: + torch.Tensor: Computed boundary loss. Shape depends on ``reduction``: + scalar if "mean" or "sum", shape (B, C') if "none" (or (C',) if batch=True). Example:🤖 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/losses/boundary_loss.py` around lines 147 - 168, The forward method in the BoundaryLoss class is missing a Returns section in its docstring. Add a Returns section after the Raises section that documents the return type as torch.Tensor and describes what it represents, such as the computed boundary loss value. This should follow the same format as the other documented parameters and exceptions in the docstring.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@monai/losses/boundary_loss.py`:
- Around line 109-124: The compute_distance_map method raises a ValueError when
the target tensor dimensions are not 4 or 5, but this exception is not
documented in the docstring. Add a Raises section to the docstring of
compute_distance_map that documents the ValueError exception along with a
description of when it is raised (specifically when target.dim() is not in (4,
5)).
- Around line 147-168: The forward method in the BoundaryLoss class is missing a
Returns section in its docstring. Add a Returns section after the Raises section
that documents the return type as torch.Tensor and describes what it represents,
such as the computed boundary loss value. This should follow the same format as
the other documented parameters and exceptions in the docstring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e42e9d2-b109-4af8-96f3-3f541d99bb04
📒 Files selected for processing (4)
docs/source/losses.rstmonai/losses/__init__.pymonai/losses/boundary_loss.pytests/losses/test_boundary_loss.py
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/source/losses.rst
- monai/losses/init.py
- tests/losses/test_boundary_loss.py
1 participant
Added BoundaryLoss. - Handles 2D/3D seg - Supports sigmoid, softmax, other_act, include_background, batch - Does empty masks without producing arbitrary distance ramps - Added tests for shape handling, reductions, gradient flow, single channel warnings, degenerative mask & channel free targets Verified with tests including run tests.sh
Closes #8884