fix(data): skip empty foreground when computing DatasetSummary statistics - #9043
Open
aymuos15 wants to merge 1 commit into
Open
fix(data): skip empty foreground when computing DatasetSummary statistics#9043aymuos15 wants to merge 1 commit into
aymuos15 wants to merge 1 commit into
Conversation
…tics Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Contributor
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 2
🤖 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/data/dataset_summary.py`:
- Around line 171-175: Complete the Google-style docstrings for
calculate_statistics and calculate_percentiles in monai/data/dataset_summary.py,
documenting their Raises behavior and Returns as None. Also add descriptive
docstrings for the mixed foreground/background and all-background error tests in
tests/data/test_dataset_summary.py at lines 103 and 124, respectively.
In `@tests/data/test_dataset_summary.py`:
- Around line 127-130: Update both assertRaisesRegex checks around
calculator.calculate_statistics() and calculator.calculate_percentiles() to
match the complete remediation text, including the exact recommendation
foreground_threshold=-1, rather than only the parameter name.
🪄 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 Plus
Run ID: 31ab911b-369d-4c2e-af71-268749401698
📒 Files selected for processing (2)
monai/data/dataset_summary.pytests/data/test_dataset_summary.py
Comment on lines
+171
to
+175
| if voxel_ct == 0: | ||
| raise ValueError( | ||
| f"no foreground voxels found in any sample with foreground_threshold={foreground_threshold}; " | ||
| "set foreground_threshold=-1 to compute statistics over whole images." | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Complete Google-style docstrings for the changed definitions.
monai/data/dataset_summary.py#L171-L175: documentRaises:andReturns: Noneforcalculate_statistics.monai/data/dataset_summary.py#L221-L224: documentRaises:andReturns: Noneforcalculate_percentiles.tests/data/test_dataset_summary.py#L103-L103: document the mixed foreground/background test.tests/data/test_dataset_summary.py#L124-L124: document the all-background error test.
As per path instructions, Python definitions must document variables, return values, and raised exceptions in Google-style docstrings.
📍 Affects 2 files
monai/data/dataset_summary.py#L171-L175(this comment)monai/data/dataset_summary.py#L221-L224tests/data/test_dataset_summary.py#L103-L103tests/data/test_dataset_summary.py#L124-L124
🤖 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/data/dataset_summary.py` around lines 171 - 175, Complete the
Google-style docstrings for calculate_statistics and calculate_percentiles in
monai/data/dataset_summary.py, documenting their Raises behavior and Returns as
None. Also add descriptive docstrings for the mixed foreground/background and
all-background error tests in tests/data/test_dataset_summary.py at lines 103
and 124, respectively.
Source: Path instructions
Comment on lines
+127
to
+130
| with self.assertRaisesRegex(ValueError, "foreground_threshold"): | ||
| calculator.calculate_statistics() | ||
| with self.assertRaisesRegex(ValueError, "foreground_threshold"): | ||
| calculator.calculate_percentiles() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the full remediation text.
The test must verify the recommendation foreground_threshold=-1. The current regex checks only foreground_threshold, so it would pass if the recommendation were removed.
Proposed assertion
- with self.assertRaisesRegex(ValueError, "foreground_threshold"):
+ with self.assertRaisesRegex(ValueError, r"set foreground_threshold=-1"):Apply this to both assertions.
📝 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.
Suggested change
| with self.assertRaisesRegex(ValueError, "foreground_threshold"): | |
| calculator.calculate_statistics() | |
| with self.assertRaisesRegex(ValueError, "foreground_threshold"): | |
| calculator.calculate_percentiles() | |
| with self.assertRaisesRegex(ValueError, r"set foreground_threshold=-1"): | |
| calculator.calculate_statistics() | |
| with self.assertRaisesRegex(ValueError, r"set foreground_threshold=-1"): | |
| calculator.calculate_percentiles() |
🤖 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/data/test_dataset_summary.py` around lines 127 - 130, Update both
assertRaisesRegex checks around calculator.calculate_statistics() and
calculator.calculate_percentiles() to match the complete remediation text,
including the exact recommendation foreground_threshold=-1, rather than only the
parameter name.
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.
Description
DatasetSummary.calculate_statisticscrashed with a bareRuntimeErroron any sample whose label had no foreground voxels at the givenforeground_threshold(common with 2D slices or sparse annotations), and if every sample lacked foreground it would divide by zero in the mean/std.calculate_percentileshad the same empty-accumulation hazard. Both methods now skip empty foreground selections and, when no sample contributes any foreground voxels, raise an actionableValueErrorthat namesforeground_thresholdand points to setting it to-1to compute statistics over whole images:monai/data/dataset_summary.py—calculate_statisticscontinues past empty foreground selections and guardsvoxel_ct == 0;calculate_percentilesskips empty selections and guards the accumulated list before callingnp.percentile.tests/data/test_dataset_summary.py— two new tests: a mixed foreground/background dataset (statistics and percentiles computed from the remaining foreground), and an all-background dataset (ValueErrorraised by both methods).Behavior verified end-to-end: a dataset with one normal and one all-background sample now produces statistics from the normal sample (previously it crashed), and an all-background dataset raises the actionable
ValueErrorfrom bothcalculate_statisticsandcalculate_percentiles.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.