FIX: Converting mm to voxels wrong for dilation in registration#37
FIX: Converting mm to voxels wrong for dilation in registration#37aylward merged 1 commit intoProject-MONAI:mainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting WalkthroughThe dilation radius computation in mask processing functions was corrected by inverting the ratio calculation. The formula now divides the requested dilation distance by voxel spacing, replacing the previous inverted ratio, affecting kernel size derivation in image registration workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Fixes the mm→voxel conversion used for dilating registration masks so that a dilation specified in millimeters is converted to an appropriate voxel radius based on image spacing.
Changes:
- Corrected mask dilation voxel-radius computation for the fixed mask.
- Corrected mask dilation voxel-radius computation for the moving mask during registration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.mask_dilation_mm > 0: | ||
| imMath = ttk.ImageMath.New(self.fixed_mask) | ||
| imMath.Dilate( | ||
| int(self.fixed_image.GetSpacing()[0] / self.mask_dilation_mm), 1, 0 | ||
| int(self.mask_dilation_mm / self.fixed_image.GetSpacing()[0]), 1, 0 | ||
| ) |
There was a problem hiding this comment.
This change fixes a unit conversion that affects mask preprocessing across all registrars, but there are no tests asserting the mm→voxel dilation behavior (search in tests found no references to mask_dilation). Adding a focused unit test that checks the computed voxel radius for a known spacing would help prevent regressions.
| if self.mask_dilation_mm > 0: | ||
| imMath = ttk.ImageMath.New(self.fixed_mask) | ||
| imMath.Dilate( | ||
| int(self.fixed_image.GetSpacing()[0] / self.mask_dilation_mm), 1, 0 | ||
| int(self.mask_dilation_mm / self.fixed_image.GetSpacing()[0]), 1, 0 | ||
| ) |
There was a problem hiding this comment.
Optional: using int(mask_dilation_mm / spacing) floors the voxel radius, so the physical dilation will be <= the requested mm (sometimes noticeably, depending on spacing). If you want the closest physical match, consider using rounding/ceiling instead (and keep the approach consistent for fixed and moving masks).
| if self.mask_dilation_mm > 0: | ||
| imMath = ttk.ImageMath.New(new_moving_mask) | ||
| imMath.Dilate( | ||
| int(moving_image.GetSpacing()[0] / self.mask_dilation_mm), 1, 0 | ||
| int(self.mask_dilation_mm / moving_image.GetSpacing()[0]), 1, 0 | ||
| ) |
There was a problem hiding this comment.
Optional: int(mask_dilation_mm / spacing) floors the voxel radius, which can under-approximate the requested physical dilation. Consider rounding/ceiling if the intention is to match mm more closely.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
=======================================
Coverage ? 13.67%
=======================================
Files ? 49
Lines ? 6596
Branches ? 0
=======================================
Hits ? 902
Misses ? 5694
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit