Repository navigation
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe percentile helper returns the maximum distance only when Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Percentile zero now selects the minimum, while None retains the maximum. No actionable merge-blocking risk is established by the supplied change context. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monai/metrics/hausdorff_distance.py (1)
207-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required Google-style docstrings to the changed definitions.
monai/metrics/hausdorff_distance.py#L207-L212: documentNoneas maximum distance,0as minimum surface distance, valid quantiles, the return value, andValueError.tests/metrics/test_hausdorff_distance.py#L236-L247: document test parameters, the return value, and the expected out-of-range error behavior.As per path instructions, Python definitions must document variables, return values, and raised exceptions in Google-style 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/hausdorff_distance.py` around lines 207 - 212, Add Google-style docstrings to the function containing the percentile-handling logic in monai/metrics/hausdorff_distance.py (lines 207-212) that document the percentile parameter behavior (None returns maximum distance, 0 returns minimum surface distance, valid quantile range), the return value (surface distance scalar), and the ValueError exception for out-of-range percentiles. Similarly, add a Google-style docstring to the test function in tests/metrics/test_hausdorff_distance.py (lines 236-247) that documents the test parameters, describes what is being validated (the return value and out-of-range error behavior), and documents the expected exception behavior when percentile values are invalid.Source: Path instructions
🤖 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/metrics/hausdorff_distance.py`:
- Around line 207-212: Add Google-style docstrings to the function containing
the percentile-handling logic in monai/metrics/hausdorff_distance.py (lines
207-212) that document the percentile parameter behavior (None returns maximum
distance, 0 returns minimum surface distance, valid quantile range), the return
value (surface distance scalar), and the ValueError exception for out-of-range
percentiles. Similarly, add a Google-style docstring to the test function in
tests/metrics/test_hausdorff_distance.py (lines 236-247) that documents the test
parameters, describes what is being validated (the return value and out-of-range
error behavior), and documents the expected exception behavior when percentile
values are invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ca73296d-7cf1-4dc0-8a19-d120f0a4fce6
📒 Files selected for processing (2)
monai/metrics/hausdorff_distance.pytests/metrics/test_hausdorff_distance.py
ericspod
reviewed
Sep 27, 2026
ericspod
added a commit
that referenced
this pull request
Oct 6, 2026
…t drop it (#9096) Fixes #9095. ### Description `HausdorffDistanceMetric(percentile=...)` returns `nan` when one of the two masks is empty, where `percentile=None` returns `inf` for the same input. `get_surface_distance` reports an infinite distance for every boundary voxel when a mask is empty, so an all-infinite tensor reaches `_compute_percentile_hausdorff_distance`. `torch.quantile` interpolates linearly between the two order statistics straddling the requested rank, and that interpolation is `inf + (inf - inf) * frac`, which is `nan`. The maximum and minimum paths escape it because they do not interpolate. The quantile of a constant sequence is that constant, so the `nan` is an artefact rather than a property of the distances. It also collides with the meaning this metric already gives `nan`: both-masks-empty returns it to say "not applicable", and `do_metric_reduction` excludes it from the average. A prediction that missed the structure entirely is therefore removed from a dataset score rather than counted as the worst case, and the reported HD95 improves as the model finds fewer structures. `get_not_nans=True` exposes the shrinking denominator but is off by default. The issue has the table. This returns the infinity directly when every distance is infinite, so the percentile path agrees with the maximum path: ```python if torch.isinf(surface_distance).all(): return torch.tensor(np.inf, dtype=torch.float, device=surface_distance.device) return torch.quantile(surface_distance, percentile / 100) ``` The guard is exact rather than defensive. `get_surface_distance` returns either all-finite or all-infinite distances and never a mixture, because the infinite branch triggers on an empty mask, which makes every distance infinite at once; each direction of the symmetric distance is computed and reduced separately, so a partly-infinite tensor does not reach this function. Tests cover both entry points, the metric class and the helper, at `percentile` None, 0, 50, 95, 99 and 100. With the source change reverted and the tests kept, eight fail and four pass, the four being None and 0, which take the non-interpolating paths. On testing: I ran `tests/metrics/` in full (393 passed, 41 skipped) and `tests/metrics/test_hausdorff_distance.py` (68 passed), plus black, isort and ruff on both changed files. One pre-existing failure in `test_compute_fid_metric.py` reproduces on unmodified `dev` with torch 2.14 and is unrelated to this change. I have not completed a full `./runtests.sh --quick --unittests` run locally, so I have left those boxes unticked rather than tick them untested. Related but independent: #9033 fixes `percentile=0` being treated as unset by the `if not percentile:` guard. It adds a `.min()` branch above the `torch.quantile` call and leaves the interpolation as it is, so the two changes do not overlap in behaviour. They touch nearby lines and whichever lands second will want a trivial rebase; happy to be the one that rebases. ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [x] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [ ] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [x] In-line docstrings updated. - [ ] Documentation updated, tested `make html` command in the `docs/` folder. --------- Signed-off-by: asifuddin01 <md.asif.uddin@g.bracu.ac.bd> Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
percentile=0 is the 0th-percentile surface distance (the minimum), but the falsy guard 'if not percentile' treated it as unset and returned surface_distance.max(). Check for None explicitly so 0 goes through the quantile branch, whose all-infinite guard keeps empty-mask cases at inf. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk> Assisted-by: Claude Opus 5.5 Signed-off-by: Soumya Snigdha Kundu <soumyawork15@gmail.com>
aymuos15
force-pushed
the
fix/hausdorff-percentile-zero
branch
from
October 7, 2026 11:28
7c5b28e to
5d55cd5
Compare
…min() Apply review suggestion: return surface_distance.min() for percentile=0 and restrict the quantile branch to 0 < percentile <= 100, and note in the docstrings that a value of 0 returns the minimum distance. Assisted-by: Claude Opus 5.5 Signed-off-by: Soumya Snigdha Kundu <soumyawork15@gmail.com>
This branch has not been deployed
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
2 participants
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.
Description
HausdorffDistanceMetric(percentile=0)returned the full Hausdorff distance instead of the 0th-percentile (minimum) surface distance. The guard in_compute_percentile_hausdorff_distancewas a truthiness test,if not percentile:, so the valid input0was treated as "percentile unset" and short-circuited tosurface_distance.max().percentileis documented as "an optional float number between 0 and 100", and the 0th percentile of the surface distances is the minimum — sopercentile=0must not be treated as unset. The fix:percentile=None→.max()(unchanged: the default, full Hausdorff distance)percentile == 0→.min()(the 0th-percentile minimum)torch.quantilebranchpercentile == 0is routed through.min()rather than the quantile branch on purpose:torch.quantilereturnsNaNfor an all-inf distance tensor (empty prediction or ground truth), which would have regressed those cases frominftoNaN.Tests: two new percentile-0 cases in the spherical-segmentation test matrix (plain and with spacing, red on the old code), plus a parameterized unit test of
_compute_percentile_hausdorff_distancecoveringNone/0/quantile paths, the all-inf guard, the empty-tensor path, and out-of-rangeValueErrors.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.