Repository navigation
Finished feature for test_set_split - #446
JoshuaSiraj merged 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable usage-based reviews in Billing to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information, and refer to the rate limits docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a nnUNet Test Set Partitioning
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/imgtools/io/nnunet_output.py (1)
311-318: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueClarify that
labelsTsis a local bookkeeping folder. nnU-Net doesn’t consume it as part of the raw dataset layout, so a short note here would keep readers from assuming it’s required or used downstream.🤖 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 `@src/imgtools/io/nnunet_output.py` around lines 311 - 318, Add a brief inline note near the dir_map setup in the nnunet output handling logic to clarify that labelsTs is only used as a local bookkeeping destination and is not part of the nnU-Net raw dataset layout consumed downstream. Keep the guidance close to the relevant logic in the split/move flow around the successful_results test_set handling so readers understand the purpose of labelsTs without changing behavior.
🤖 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 `@src/imgtools/cli/nnunet_pipeline.py`:
- Around line 126-130: The help text for the --test-set-ratio option in the
click option definition is misleading because split_dataset() applies the ratio
to processed samples (successful_results), not raw image files. Update the help
string near the CLI option in nnunet_pipeline.py to describe the split as being
based on processed samples/output results rather than “valid images,” keeping
the wording aligned with the actual behavior of split_dataset().
In `@src/imgtools/io/nnunet_output.py`:
- Around line 313-315: The split logic in finalize_dataset() can place every
successful sample into test_set, leaving imagesTr empty and breaking training.
Adjust the test_set selection so it always leaves at least one successful case
in the training split, by capping the sampled test count below success_count
when success_count is nonzero. Use the existing success_count, test_set_ratio,
and random_sample logic in finalize_dataset() to implement the safeguard.
---
Nitpick comments:
In `@src/imgtools/io/nnunet_output.py`:
- Around line 311-318: Add a brief inline note near the dir_map setup in the
nnunet output handling logic to clarify that labelsTs is only used as a local
bookkeeping destination and is not part of the nnU-Net raw dataset layout
consumed downstream. Keep the guidance close to the relevant logic in the
split/move flow around the successful_results test_set handling so readers
understand the purpose of labelsTs without changing behavior.
🪄 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: e9b0dc9e-6924-4926-a133-2400b3b97c1b
📒 Files selected for processing (3)
src/imgtools/cli/nnunet_pipeline.pysrc/imgtools/io/nnunet_output.pysrc/imgtools/nnunet_pipeline.py
| @click.option( | ||
| "--test-set-ratio", | ||
| type=click.FloatRange(0.0,1.0), | ||
| default=0.0, | ||
| help="The proportion of valid images that are to be used in the test set.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the help text with the actual split unit.
split_dataset() applies the ratio to processed samples (successful_results), not individual image files, so this wording is misleading when one sample produces multiple outputs.
Suggested change
- help="The proportion of valid images that are to be used in the test set.",
+ help="The proportion of successfully processed samples to place in the test set.",📝 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.
| @click.option( | |
| "--test-set-ratio", | |
| type=click.FloatRange(0.0,1.0), | |
| default=0.0, | |
| help="The proportion of valid images that are to be used in the test set.", | |
| `@click.option`( | |
| "--test-set-ratio", | |
| type=click.FloatRange(0.0,1.0), | |
| default=0.0, | |
| help="The proportion of successfully processed samples to place in the test set.", |
🤖 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 `@src/imgtools/cli/nnunet_pipeline.py` around lines 126 - 130, The help text
for the --test-set-ratio option in the click option definition is misleading
because split_dataset() applies the ratio to processed samples
(successful_results), not raw image files. Update the help string near the CLI
option in nnunet_pipeline.py to describe the split as being based on processed
samples/output results rather than “valid images,” keeping the wording aligned
with the actual behavior of split_dataset().
| success_count = len(successful_results) | ||
| test_set = random_sample(successful_results, ceil(test_set_ratio * success_count)) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep at least one case in imagesTr.
Using ceil(test_set_ratio * success_count) can move all successful samples into the test split. For example, success_count == 1 with any positive ratio yields 1, and test_set_ratio == 1.0 yields success_count. finalize_dataset() then computes num_training_cases from an empty imagesTr, which makes the generated dataset unusable for training.
Suggested change
success_count = len(successful_results)
- test_set = random_sample(successful_results, ceil(test_set_ratio * success_count))
+ if success_count < 2 or test_set_ratio <= 0:
+ return
+
+ requested_test_count = ceil(test_set_ratio * success_count)
+ test_count = min(requested_test_count, success_count - 1)
+ test_set = random_sample(successful_results, test_count)📝 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.
| success_count = len(successful_results) | |
| test_set = random_sample(successful_results, ceil(test_set_ratio * success_count)) | |
| success_count = len(successful_results) | |
| if success_count < 2 or test_set_ratio <= 0: | |
| return | |
| requested_test_count = ceil(test_set_ratio * success_count) | |
| test_count = min(requested_test_count, success_count - 1) | |
| test_set = random_sample(successful_results, test_count) |
🤖 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 `@src/imgtools/io/nnunet_output.py` around lines 313 - 315, The split logic in
finalize_dataset() can place every successful sample into test_set, leaving
imagesTr empty and breaking training. Adjust the test_set selection so it always
leaves at least one successful case in the training split, by capping the
sampled test count below success_count when success_count is nonzero. Use the
existing success_count, test_set_ratio, and random_sample logic in
finalize_dataset() to implement the safeguard.
Hey this looks great! I'll put some time aside to put together some tests for the full nnunet-pipeline + these changes and merge ASAP. Thank you again for your help! |
2c174e7
into
bhklab:JoshuaSiraj/nnunet_updates
Hi sorry issue took a little while, but I wrote up a minimal way of implementing a test_set_split for nnunet-pipeline.
The feature takes the successfully processed images and moves a proportion of the images/labels to the appropriate dirs in the output dir. I could not find any tests for nnunet-pipeline but this should probably be added to them when they're ready. There is CLI support for the feature as well as hints. Please let me know if there's anything II should change style wise.
Fixes #443
Summary by CodeRabbit
New Features
test-set-ratiooption to the command-line workflow, letting you split a portion of successfully processed data into a test set.Bug Fixes