-
Notifications
You must be signed in to change notification settings - Fork 181
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
add mrgrid #1609
add mrgrid #1609
Conversation
…op, mrpad and mrtransform (identity). Added template and datatype options for all modes. #364
…ot supported yet)
… bounds value. added regrid adapter with default value.
…tation. resize filter: bugfix out of bounds value
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.
Looks very good to me; I'm very happy with the choices made, and also fully support separate -template
and -as
options: even though they're both supplying the image that serves as a "reference", there's an important difference in what aspects are used (or ignored) as reference information.
One thing that I picked up from the help/interface (and didn't fully anticipate) is that -as
will perform a right pad or crop. I think what I'd naturally had been expecting first would be more of a "centered" pad or crop. But in saying that, I now realise that'll introduce the necessity to deal with an ambiguity if the input image and -as
reference image differ by an uneven number along a(ny) dimension... So taking that then into account, I think right pad or crop is probably fine.
Happy to delete the commands that this effectively replaces: no functionality is lost (I think).
|
…stances in lib/mrtrix3 with equivalent mrgrid commands. user docs updated. mask crop tighter by 1 voxel.
done.
done, documented the change in the
Yes, the operation can be inverted (pad instead of crop and vice versa) by flipping the sign in the delta specifier. I made that more explicit in the
done, added description of
To concatenate multiple SH images with varying lmax in the 4th dimension. This requires zero-padding to the largest lmax prior to concatenation.
All operations in mrgrid preseve the location in scanner reference frame. Another reason for right-padding is that it also preserves the image transformation. |
Yep, thought that was part of the intention behind that choice. I'm happy with it; it makes sense, I reckon.
Yep, was thinking of the same. I have a few other uses in mind, but don't wish to elaborate on those here. There's definitely some uses, and I'm sure users will find their own. Fully support having the option; having options is a good thing. |
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.
Review-refresh: all still good.
Just comes to mind now: when removing the older commands, you'll have to fix up the tests, I suppose. But as the new command basically allows for the same functionality, you could re-write the tests as such that the test data doesn't have to be touched. That itself would be a good test indeed. |
- Fix execution of mrgrid command in population_template (error from #1609). - Fix app.error() -> raise MRtrixError merge error in dwi2response (error in #1614). - For app.add_dwgrad_*_options(), pass command-line parser as argument, since it may be a subparser rather than app.CMDLINE to which the options are added.
- mrthreshold: Fix erroneous exception message introduced in #1636. - dwi2response msmt_5tt: Compatibility fix for low-resolution data (ie. few voxels), when running dwi2response tournier, based on new mrthreshold -top implementation in #1636. - 5ttgen: Fixes to FoV cropping due to behaviour of mrgrid crop, which deviated from former command mrcrop in #1609 but was restored to previous behaviour in #1678.
- Add missing copyright statement. - Move some documentation on future improvements / fixes to head of text file. - Pre-convert aparc image in order to prevent repeated uncompression. - Update final cropping step to use mrgrid command (#1609). - Issue warning at completion of script regarding absence of AC segmentation. - Generate additions to 5ttgen command documentation.
Adding
mrgrid
as a replacement tomrcrop
,mrpad
andmrresize
(#364).The syntax follows closely what @jdtournier suggested in this comment with the exception of the distinction between
-template
and-as
options. As inmrtransform
, the former indicates that interpolation is performed (regrid
operation), the latter doesn't (crop
orpad
).The added
Regrid
adapter as a generalisation of theSubset
adapter that allows padding with fill values. However, the bounds checks inmove_index
andvalue
feel clunky. Is there a better way?Are we happy with the interface?
Shall I remove the existing commands?
The help page: