Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Excessive padding when resampling zooms #696

Open
mgxd opened this issue Mar 17, 2022 · 3 comments
Open

Excessive padding when resampling zooms #696

mgxd opened this issue Mar 17, 2022 · 3 comments

Comments

@mgxd
Copy link
Contributor

mgxd commented Mar 17, 2022

When working with RegridToZooms (aka resample_by_spacing), I was going from even -> odd slice lengths when doubling the zooms of an image. After a little digging, it seems to me we should be rounding (maybe to the 3rd decimal spot) before calling np.ceil to avoid any extra slices..

Relevant code section:

card = nb.affines.from_matvec(np.diag(pre_zooms))
extent = card[:3, :3].dot(np.array(in_file.shape[:3]))
card[:3, 3] = -0.5 * extent
# Cover the FoV with the new grid
new_size = np.ceil(extent / zooms).astype(int)

Test case:

ipdb> card
array([[0.8, 0. , 0. , 0. ],
       [0. , 0.8, 0. , 0. ],
       [0. , 0. , 0.8, 0. ],
       [0. , 0. , 0. , 1. ]], dtype=float32)
ipdb> pre_zooms
array([0.8, 0.8, 0.8], dtype=float32)
ipdb> in_file.shape[:3]
(208, 300, 320)
ipdb> extent
array([166.40000248, 240.00000358, 256.00000381])
ipdb> zooms
array([1.6, 1.6, 1.6])
ipdb> extent / zooms
array([104.00000155, 150.00000224, 160.00000238])
ipdb> new_shape
array([105, 151, 161])
# I would expect a shape of [104, 150, 160] here

Does this make sense, or is it fine to just include the extra slice? cc @oesteban

@effigies
Copy link
Member

effigies commented Mar 17, 2022

That seems reasonable to me.

Just to derail a little bit, this function worries me. It's a complicated, one-off procedure that implements a lot of things that should be composable operations. It seems like it would be better implemented as:

def resample_to_zooms(img, target_zooms, **kwargs):
    shape, affine = rezoom(img.affine, target_zooms)
    new_img = resample_img(img, shape, affine, **kwargs)
    # Possibly do some header fiddling
    return new_img

def resample_img(img, shape, affine, order=3):
    ...

def rezoom(affine, target_zooms):
    ...

Then we could even parameterize rezoom to determine what happens in these edge cases, rather than opaquely adjusting a call to np.ceil(). Or make it clear in the docstring that resample_to_zoom will always round up and that the caller should check the expected shape and possibly use img.slicer[:-1, :-1, :-1] to get the desired shape.

@mgxd
Copy link
Contributor Author

mgxd commented Mar 17, 2022

agreed - it will also simplify testing

@oesteban
Copy link
Member

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants