-
Notifications
You must be signed in to change notification settings - Fork 27
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
ENH: Integrate downsampling in BSplineApprox
when the input is high-res
#301
Conversation
cc/ @eilidhmacnicol |
BTW, this is meant to be merged before the other PRs |
Adds a new input ``zooms_min`` that will trigger the downsampling of the input field map when it comes with a very high resolution (typically this is the case for SDC SyN). Although the input is resampled, the output field is interpolated at the original resolution for debugging purposes. Downsampling dramatically speeds up approximation. Alternatively, a solution (for the SDC SyN) would be to calculate a heavily dilated mask of the brain, and use it to limit the number of voxels that are fit in regression.
679f09a
to
b7dd98b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Architectural changes look good. I think we need a different threshold and I would incline toward a simpler down-sampling method.
It doesn't necessarily need to be binary. I didn't respond to your concerns
about the niworkflows function. Happy to use something simpler with less
issues.
…On Wed, Nov 16, 2022, 17:32 Chris Markiewicz ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In sdcflows/interfaces/bspline.py
<#301 (comment)>:
> + if isdefined(self.inputs.in_mask)
+ else None
+ )
+
+ need_resize = np.any(np.array(zooms) < self.inputs.zooms_min)
+ if need_resize:
+ from niworkflows.utils.images import resample_by_spacing
+
+ LOGGER.info(
+ "Resampling image with resolution exceeding 'zooms_min' "
+ f"({'x'.join(str(s) for s in zooms)})."
+ )
+ fmapnii = resample_by_spacing(fmapnii, [self.inputs.zooms_min] * 3)
+
+ if masknii is not None:
+ masknii = resample_by_spacing(masknii, [self.inputs.zooms_min] * 3)
I thought the mask was binary. I'll need to look again.
—
Reply to this email directly, view it on GitHub
<#301 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAESDRVNJUFBGOKRAPVYG3LWIUEC3ANCNFSM6AAAAAASCDN2UM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes - any hope for a few tests (with / without downsampling)?
Allows anisotropic voxel sizes when resampling the input data. cc/ @eilidhmacnicol Co-authored-by: Chris Markiewicz <[email protected]>
Replace the controverted function with a new implementation, which could be condiered as a candidate to replace Niworkflows'. Co-authored-by: Chris Markiewicz <[email protected]> Co-authored-by: Mathias Goncalves <[email protected]>
Co-authored-by: Mathias Goncalves <[email protected]>
LMKWYT |
sdcflows/utils/tools.py
Outdated
hdr.set_sform(affine, scode) | ||
hdr.set_qform(affine, qcode) | ||
newref = in_file.__class__( | ||
np.zeros(new_shape, dtype=hdr.get_data_dtype()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a big deal, but I don't think the dtype matters, but setting it so precisely implies that it does. This might make one hesitant to change things in the future. Just flagging the thought that we might want some way to indicate what's important, like SpatialReference(shape, affine)
, but should not hold up this PR if there isn't something that simple already in nitransforms.
) | ||
|
||
# Resample via identity transform | ||
return Affine(reference=newref).apply(in_file, order=order, prefilter=prefilter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at scipy.ndimage.map_coordinates, I don't think prefilter filters more or less based on the down-sampling factor.
I suspect for the factors we'll be dealing with (mostly <2, almost all <3) it's probably fine, but I don't really know how to evaluate this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at scipy.ndimage.map_coordinates, I don't think prefilter filters more or less based on the down-sampling factor.
No, that's the b-spline filtering, which blurs with the width of the B-Spline basis (cubic, then 4 voxels). I believe that is enough for the purpose of this interpolation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of SyN, ANTs is already giving you a displacements field that has been smoothed to a certain kernel width (the third parameter of -t [Syn]). We are not in a more general case where you have a signal with additive noise and you want to subsample it safely.
So I'm quite convinced no extra smoothing should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(to answer the question, you are right, it is independent of the size ratio) -- but that should be okay, IMHO, for the reasons above.
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
I'll go ahead and merge. I plan to make a PR collecting post-merge comments, so, if something new appears, I will address it later. |
if need_resize: | ||
from sdcflows.utils.tools import resample_to_zooms | ||
|
||
zooms_min = np.maximum(zooms, self.inputs.zooms_min) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarity, I think we want to rename from zooms_min
, since it now diverges form self.inputs.zooms_min
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would you suggest? (I have no particular preference)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target_zooms
made sense to me.
Adds a new input
zooms_min
that will trigger the downsampling of the input field map when it comes with a very high resolution (typically, this is the case for SDC SyN).Although the input is resampled, the output field is interpolated at the original resolution for debugging purposes.
Downsampling dramatically speeds up approximation. Alternatively, a solution (for the SDC SyN) would be to calculate a heavily dilated mask of the brain and use it to limit the number of voxels that are fit in regression.