Fix: Prevent negative indices in downsample_fft when image dimension is less than MIN_IMG_SIZE#44
Fix: Prevent negative indices in downsample_fft when image dimension is less than MIN_IMG_SIZE#44leonardodtang wants to merge 7 commits intorohitrango:mainfrom
downsample_fft when image dimension is less than MIN_IMG_SIZE#44Conversation
…pendency. Allows for smaller minimum image sizes
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the downsample_fft function where negative indices occur when image dimensions are smaller than MIN_IMG_SIZE (default 32). The issue commonly arises with medical images like prostate MRI that have small slice counts.
- Replaces hardcoded
MIN_IMG_SIZEwith adaptivemin_dimcalculation that finds the largest power-of-2 dimension that fits the image - Adds minimum dimension validation with
MIN_IMG_SHARDED_SIZEas the lower bound - Temporarily fixes interpolation mode issue in
FakeBatchedImages
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| fireants/registration/abstract.py | Adds _set_min_dim() method to calculate adaptive minimum dimensions and fixes interpolation mode |
| fireants/registration/syn.py | Replaces MIN_IMG_SIZE with self.min_dim in optimization loop |
| fireants/registration/rigid.py | Replaces MIN_IMG_SIZE with self.min_dim for both fixed and moving image downsampling |
| fireants/registration/moments.py | Updates downsampling logic to use self.min_dim instead of MIN_IMG_SIZE |
| fireants/registration/greedy.py | Replaces MIN_IMG_SIZE with self.min_dim in image resizing calculations |
| fireants/registration/deformation/svf.py | Implements standalone min_dim_f calculation for non-AbstractRegistration class |
| fireants/registration/deformation/compositive.py | Adds adaptive minimum dimension logic similar to SVF implementation |
| fireants/registration/affine.py | Updates downsampling to use self.min_dim instead of hardcoded constant |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…ct.py and logic in svf.py, removed duplication in compositive.py
|
looks good to me! one last thing, can you add a unittest checking for this behavior? You can use synthetic images for this, we just want to check if the min image size is correct for different sizes of images that might be possible. Thanks for your contribution! |
Signed-off-by: leonardodtang <35482231+leonardodtang@users.noreply.github.com>
|
Added |
| self.loss_fn.set_scales(self.scales) | ||
|
|
||
| # set min dim for img_size | ||
| fixed_arrays = self.fixed_images() |
There was a problem hiding this comment.
do we need to get fixed_arrays and moving_arrays? There is a shape attribute of the BatchedImages object that you can use directly
fireants.utils.imageutils.downsample_fft(line 163): When the global variableMIN_IMG_SIZE(default 32) is larger than one of the image's original dimensions, the calculatedtarget_dimscan be smaller thansource_dims. This results in negative values within thestart_idxarray, which subsequently causes indexing to fail.This is a common issue when processing non-brain MRI with high slice thickness and a small FOV, such as prostate MRI with imagedimensions of 20x192x192 (voxel size 3.5x1.35x1.35mm).
A quick fix would be to simply lower the
MIN_IMG_SIZEto 16 or 8, but this should come with some caveats to limit the slice thickness to reasonable numbers.Changes:
AbstractRegistrationis modified to create aself.min_dimvariable.self.min_dimis set withself._set_min_dimduring initialization. In classes that inherit fromAbstractRegistration,self.min_dimis used in place ofMIN_IMG_SIZEMIN_IMG_SHARDED_SIZE, currently set to 4. Can modify lower bound to something other thanMIN_IMG_SHARDED_SIZEif 4 is too small.CompositiveWarpandStationaryVelocitydo not inherit fromAbstractRegistrationbut useMIN_IMG_SIZE, soMIN_IMG_SIZEhas been replaced withmin_dim_f, calculated the same way as inAbstractRegistration._set_min_diminterpolate_modeforFakeBatchedImagesinfireants.registration.abstract(line 336) appears to be broken, reverting to bilinearfireants.registration.greedy.GreedyRegistration.get_inverse_warp_parameterserroneously refers tofixed_arrayswhich is not initialized (line 168). Changed tofixed_imagesTested affine and greedy registration on images with 24 and 20 slices. More testing may be necessary