MRtrix3 / mrtrix3

MRtrix3 provides a set of tools to perform various advanced diffusion MRI analyses, including constrained spherical deconvolution (CSD), probabilistic tractography, track-density imaging, and apparent fibre density
http://www.mrtrix.org
Mozilla Public License 2.0
282 stars 177 forks source link

New command - mrgrid #364

Closed draffelt closed 5 years ago

draffelt commented 8 years ago

As discussed originally in Gitlab, we decided that mrresize should be incorporated into mrtransform (#15). Even though this does not strictly transform the image, it was decided that it should be ok to have the mrresize options (-scale, -voxel, -size) as a part of the "regridding options" in mrtransform.

However, given that we also have mrcrop and mrpad, I'm wondering if it makes more sense to have a single mrgrid command. We could then have separate option groups for crop, pad, and resize.

This would also mean that we avoid having to hide the mrresize options to mrtransform (which is already a fairly large command).

What do you think?

thijsdhollander commented 8 years ago

Yes, I think this is a great idea. Now that I started thinking about it, mrresize was actually a bit deceiving: it sounds like it will (allow to) scale the actual images, i.e., as in actually transforming them. A command named mrgrid to basically just map an image to a new grid will avoid that confusion.

Maybe within the context of such a command, it might be worth to "generalize" some cropping and padding options, i.e., turning them into more unified "field of view" options (if that makes sense).

jdtournier commented 8 years ago

Happy enough with all of the above - although I'm still generally of the opinion that these operations are all sufficiently similar to justify being part of a single command. I can see that there is a subtle distinction: one set of operations doesn't modify the position of the underlying object in real space or the axes used for the grid (mrgrid), the other set does (mrtransform). It's just a matter of whether that distinction is sufficient to warrant a different command.

I'll go through my arguments again, and you can happily ignore me if you still disagree, happy to go with the majority vote on that one.

First off, I would expect that all of the various modes of operation offered by mrgrid will also need to be available within mrtransform, so that invoking mrtransform using an identity transform and appropriate options should allow you to do everything that mrgrid does. Maybe that might require tweaking an existing image (and maybe that's what mrgrid might allow you to do) to pass to mrtransform's -template option, but the point is it should all be feasible out of the box. So the only functionality to add to mrtransform would be options to somehow modify the template image provided - the actual regridding code would be more or less unmodified.

In terms of actual code, I'm also thinking that merging makes sense. If mrgrid only offered the '-crop' & -pad options, there would be no interpolation required, so it would make sense to have a tiny app that does just that. But as soon as you offer the -scale option, you need to interpolate, and that means bringing in the whole interpolator backend and all the options that relate to that. So the mrgrid command would need to have a additional options and code already in mrtransform.

On the other hand, I can see that having it all in one command would complicate mrtransform a little, particularly in terms of setting up the template image and figuring out whether to use the interpolation branch or the straight copy branch (although that shouldn't be all that difficult). Really, the various functions offered by mrgrid relate to modifying the target grid of the input image without reorienting its axes. I think such options would potentially be useful in mrtransform (e.g. regridding to template image upsampled by factor 2), but maybe the simplest/cleanest thing is to have a command that explicitly allows you to modify that template image (i.e. mrgrid), which you can then pass to mrtransform's -template option. So from that point of view, if you prefer that type of UX, why not? Personally I think it's a bit overkill to have to generate an image that will only be used as template for some other operation, but it's pretty minor.

So that's my 2 cents...

draffelt commented 8 years ago

I agree that some of the code paths would be the same if incorporated into mrtransform. However, I don't think there is a lot of duplication. I guess that's the reason why we created convenience functions like filter::reslice in the first place. My preference is for a separate mrgrid since it is more obvious for a user to find. As a new user, I don't think mrtransform would be the first place I would look (in Gimp all transform functions are grouped separate from scale and canvas size). And then if a user does think to look in the mrtransform help, they still need to wade through a large help page. I think combining functions to reduce the number of commands makes sense, but not if it means a user has to read through multiple large help files to find what they are after.

On Mon, Sep 28, 2015 at 6:53 PM, J-Donald Tournier <notifications@github.com

wrote:

Happy enough with all of the above - although I'm still generally of the opinion that these operations are all sufficiently similar to justify being part of a single command. I can see that there is a subtle distinction: one set of operations doesn't modify the position of the underlying object in real space or the axes used for the grid (mrgrid), the other set does ( mrtransform). It's just a matter of whether that distinction is sufficient to warrant a different command.

I'll go through my arguments again, and you can happily ignore me if you still disagree, happy to go with the majority vote on that one.

First off, I would expect that all of the various modes of operation offered by mrgrid will also need to be available within mrtransform, so that invoking mrtransform using an identity transform and appropriate options should allow you to do everything that mrgrid does. Maybe that might require tweaking an existing image (and maybe that's what mrgrid might allow you to do) to pass to mrtransform's -template option, but the point is it should all be feasible out of the box. So the only functionality to add to mrtransform would be options to somehow modify the template image provided - the actual regridding code would be more or less unmodified.

In terms of actual code, I'm also thinking that merging makes sense. If mrgrid only offered the '-crop' & -pad options, there would be no interpolation required, so it would make sense to have a tiny app that does just that. But as soon as you offer the -scale option, you need to interpolate, and that means bringing in the whole interpolator backend and all the options that relate to that. So the mrgrid command would need to have a additional options and code already in mrtransform.

On the other hand, I can see that having it all in one command would complicate mrtransform a little, particularly in terms of setting up the template image and figuring out whether to use the interpolation branch or the straight copy branch (although that shouldn't be all that difficult). Really, the various functions offered by mrgrid relate to modifying the target grid of the input image without reorienting its axes. I think such options would potentially be useful in mrtransform (e.g. regridding to template image upsampled by factor 2), but maybe the simplest/cleanest thing is to have a command that explicitly allows you to modify that template image (i.e. mrgrid), which you can then pass to mrtransform's -template option. So from that point of view, if you prefer that type of UX, why not? Personally I think it's a bit overkill to have to generate an image that will only be used as template f or some other operation, but it's pretty minor.

So that's my 2 cents...

— Reply to this email directly or view it on GitHub https://github.com/MRtrix3/mrtrix3/issues/364#issuecomment-143682553.

Lestropie commented 8 years ago

Also related to #16.

I think having mrcrop and mrpad together makes sense; just expanding / contracting the FoV without requiring any interpolation. But agree that introducing -resize into such a command blurs the lines between it and mrtransform.

Then again, I find the existing mrtransform a little unusual at times; you can just apply / replace the transform in the header, or you can do a complete re-gridding based on a new voxel size (which barely affects the 'transform' at all) and/or a template image. I sometimes wonder if mrtransform should be reserved for affecting the header transform matrix only, whereas an mrreslice should be responsible for resampling onto a new grid (which includes both template image and voxel size options). But from memory this doesn't fit well with the requirements for non-linear / FOD registration...?

draffelt commented 8 years ago

I think mrtransform should be reserved purely for transforming images wrt to scanner coordinates. If you can do that without re-gridding by applying it directly to, or replacing, the header, then that's just a bonus.

If anything, we should remove the ability for mrtransform to regrid without a -linear transform (i.e. by supplying a -template image grid only) and move this capability into mrgrid along side resize and fov operations. Since all of these operations just change the grid...not the image position wrt to scanner coordinates (note I'm not that opposed to just leaving it in mrtransform as well and duplicating functionality).

I think just because both of these commands happen to interpolate (and therefore use Filter::relsice) and may change the voxel size is irrelevant to the user.

The only good argument I can see for including -resize into mrtransform is so that you can conveniently up sample while transforming. However, as Donald said this can just be done in two steps by upsampling the template image first, which is more in line with the "unix philosophy".

On Tue, Sep 29, 2015 at 10:27 PM, Robert Smith notifications@github.com wrote:

Also related to #16 https://github.com/MRtrix3/mrtrix3/issues/16.

I think having mrcrop and mrpad together makes sense; just expanding / contracting the FoV without requiring any interpolation. But agree that introducing -resize into such a command blurs the lines between it and mrtransform.

Then again, I find the existing mrtransform a little unusual at times; you can just apply / replace the transform in the header, or you can do a complete re-gridding based on a new voxel size (which barely affects the 'transform' at all) and/or a template image. I sometimes wonder if mrtransform should be reserved for affecting the header transform matrix only, whereas an mrreslice should be responsible for resampling onto a new grid (which includes both template image and voxel size options). But from memory this doesn't fit well with the requirements for non-linear / FOD registration...?

— Reply to this email directly or view it on GitHub https://github.com/MRtrix3/mrtrix3/issues/364#issuecomment-144044269.

thijsdhollander commented 8 years ago

Fully agree with @draffelt here. It's not about what functionality happens to be used to accomplish the task, it's about the task you offer.

mrtransform , as the name suggests, should be there to transform images. That may be pulled off just using the header if possible, or reslice if needed. Just regridding/cropping/padding without transforming... is simply not transforming. Makes sense, right? :-)

mrgrid , again as the name suggests, does not transform the image, it just redefines the grid. If the new requested grid somehow aligns with, and has the same voxel size as, the old one; that ends up being a crop/pad type of operation where reslicing could potentially be avoided. Otherwise, it would again reslice if needed.

I mean, why do we put so much effort in coming up with good command names, if they end up not reflecting what the command does? You'll end up confusing/misleading users, and we'll get the same typical questions over and over again on the support fora. If the command name can lead the user to the right command, and from there on the help takes them further to the right options; you have software that works for the users (and provides inherent support upfront).

The logic of pulling apart stuff based on what code/functionality is used, does not make a lot of sense, I find. If you reason that logic through, the concept of the apps becomes useless at some point; since about every app would end up being a very light wrapper around just one (or a small group) of back end API calls. IMO, the apps are there to make clever use of the API to accomplish higher level tasks that make sense to the workflow of actual real users. The fact that we have convenience functions such as the Filter::reslice (and about any filter, for that matter), makes that such apps can be written using minimal duplication of actual low level algorithms. Our building blocks are (convenience) API calls; the end user's building blocks are tasks. There's an important difference there; and encapsulation/abstraction allows us to make that happen. :-)

Lestropie commented 8 years ago

I think if the name were changed from mrgrid to mrfov, then just having pad & crop options in a lightweight app makes more sense. But it doesn't answer where simple up/down-sampling should go, and mrtransform to me seems an unusual place for a user to look in that scenario. Hence why I thought reducing the scope of mrtransform and having an mrreslice might work; both up/down-sampling and resampling to match a template image belong together as 'reslicing'. With mrgrid, that would be a reasonable place to look for resampling an image onto the grid of another image.

Sounds like Dave & Thijs's train of thought is similar, but having what I'm calling mrfov and mrreslice together as mrgrid? Not against that; 'FoV' / 'reslice' is perhaps more aligned with other software packages' terminology, but if it encapsulates functionality in a powerful way, and is still discoverable to the user, then it's all good by me.

draffelt commented 8 years ago

I'm not opposed to splitting mrgrid into two smaller commands mreslice, and mrfov (I proposed mrfov a while back but did not get any takers). However, to me mrreslice also sounds like you are reslicing and image into the grid of another (and not resizing the resolution).

If we really want to reduce the number of commands I think mrgrid would be ok, however I can see why it's not ideal since grid is verb. We could have mregrid. I guess you could argue extending or reducing the grid extent is still 'regridding'.

On Wed, Sep 30, 2015 at 11:56 AM, Robert Smith notifications@github.com wrote:

I think if the name were changed from mrgrid to mrfov, then just having pad & crop options in a lightweight app makes more sense. But it doesn't answer where simple up/down-sampling should go, and mrtransform to me seems an unusual place for a user to look in that scenario. Hence why I thought reducing the scope of mrtransform and having an mrreslice might work; both up/down-sampling and resampling to match a template image belong together as 'reslicing'. With mrgrid, that would be a reasonable place to look for resampling an image onto the grid of another image.

Sounds like Dave & Thijs's train of thought is similar, but having what I'm calling mrfov and mrreslice together as mrgrid? Not against that; 'FoV' / 'reslice' is perhaps more aligned with other software packages' terminology, but if it encapsulates functionality in a powerful way, and is still discoverable to the user, then it's all good by me.

— Reply to this email directly or view it on GitHub https://github.com/MRtrix3/mrtrix3/issues/364#issuecomment-144256931.

jdtournier commented 8 years ago

Guess I'm outvoted on this one. No problem, I think we should take the consensus approach. However, since the debate is ongoing, I'll keep adding my 2 cents, see if anyone comes round to my point of view - call me stubborn...

So as I've said before, I've no major objections to having these different commands in there. It's just that I don't think they're necessary, will duplicate functionality, increase the amount of code to maintain, and won't necessarily be any more obvious to users.

The first few points are not necessarily a great argument if our aim is purely to make MRtrix3 the most user-friendly and awesome package out there, at all costs. The problem is that this would be fine if we had an army of developers and all the time in the world. We don't, so I think our time is better spent making sure that the few commands we do provide do their job as well and as flexibly as possible. As @draffelt kindly reminded me (the irony is not lost on me BTW - git), these commands should do one job and do it well. I think the difference in opinion here is what qualifies as 'the job'. As far as I can tell, mrtransform needs to be able to do all that we want to achieve with these other commands, so why would you duplicate?

In more detail: the reason I still favour combining all this into mrtransform is that the way I see it, there are three aspects that the user has (or should) have control over:

So for me, these other apps basically allow you to specify point 3: the target grid. The other two are left at default values (no transformation, default interpolator, which could be nearest-neighbour if the app determines no actual resampling is necessary). I reckon if I was knowledgeable in this field and came to MRtrix3 to find you had one app to do all this, and other apps to perform the simpler operations, I might actually find this confusing: why two apps to achieve the same thing? Even the syntax wouldn't necessarily be hugely more convenient...

As I see it, the main argument for having these other apps is one of discoverability: how easy is it for a new - but savvy - user to figure out how to do these things. Sure, he might go through the list of commands and notice the other apps. On the other hand, he might notice mrtransform first (or have used it before), and figure out that this should also do the job, since modifying the grid or upsampling should inherently be possible with any app that claims to apply a transformation like this.

The other way of looking at this discoverability issue is that commands should be task-focused to aid discoverability, as @thijsdhollander suggested. Padding, cropping, or upsampling an image are different tasks from applying a transformation, so we should provide different apps to do these. I'm not sure I buy this approach. Often, different commands can be used to do the same task, sometimes a specific task can be performed using a command that was not explicitly designed for this task, and many times a seemingly simple task requires a number of commands. For instance, just this morning I was looking at a dataset where the slices were all out of order - It's easy to fix using mrconvert's -coord option, but it is definitely far from obvious that you could do this... All this to say that trying to provide for all possible tasks is a losing battle... I would rather apps were focused on their functionality, and users can figure out for themselves whether that functionality can help them perform the specific tasks they need to do. While we may get the odd query about how to do such things on the mailing list, I don't think it's going to be such a big issue - it's not like we've had a lot of queries of that nature in the past... I would prefer that we have a 'tasks' section on the wiki, where we can go through some of these tasks and outline how they can be done (bearing in mind many of these will require several commands anyway).

So that's all I had to say. Like I said, happy to leave the final decision up to you guys, but I just thought I would clarify my position as much as possible while we're talking about it...

draffelt commented 8 years ago

So I'm leaning towards a compromise of only having one mrregrid command (instead of both mrfov and mrreslice) for the reasons I've mentioned before, and the comments below.

As far as I can tell, mrtransform needs to be able to do all that we want to achieve with these other commands

I disagree with this point. The functionality of mrtransform only needs to be able to transform an image (see my next comment).

So as I've said before, I've no major objections to having these different commands in there. It's just that I don't think they're necessary, will duplicate functionality, increase the amount of code to maintain, and won't necessarily be any more obvious to users.

As I mentioned before. We could ensure there is no overlap in the functionality by enforcing that mrtransform actually transforms an image (i.e. the user must supply either -linear or -warp or both). We can then reserve regridding onto another template image grid (without a transform) for mrregrid. In all other packages I've used the transformation command requires at least one transform.

The first few points are not necessarily a great argument if our aim is purely to make MRtrix3 the most user-friendly and awesome package out there, at all costs. The problem is that this would be fine if we had an army of developers and all the time in the world. We don't, so I think our time is better spent making sure that the few commands we do provide do their job as well and as flexibly as possible.

I fully agree we need to keep the code as small as possible for maintenance reasons. However, I don't think the cost is that high for this one (we have spent more time discussing this than it will take to write and maintain a mrregrid command). Mrtransform is becoming quite a beast with lots of different options and code paths (and it will get bigger once a few more symmetric registration related features have been added). If anything, splitting out the gridding options actually makes the code easier to read and test (fewer different code paths). Also, there are quite a few options in mrcrop, mrpad and mrresize that would bloat the mrtransform help page (I hate sifting through long man pages just to find the option I'm after). To have complete flexibility we would need to have separate -uniform and -axis options for both crop and pad. I'd prefer to have mrtransform just have options related to transforming an image. Also theoretically the MRtrix API should remain fairly stable now so the few extra lines required to have a separate mrregrid command will not need to be ported for a long time.

As @draffelt kindly reminded me (the irony is not lost on me BTW - git), these commands should do one job and do it well.

image

jdtournier commented 8 years ago

:grin: I guess we'll have to disagree then - especially on that last point... :wink:

Anyway, I've made my case, and I said I'd shut up, so I will. Up to you guys how you take it from here - ultimately you'll be using more than I will, so it's important this works for you...

jdtournier commented 8 years ago

Sorry, just one one thing: if you remove the ability to use mrtransform without a transform, and mrregrid only allows pad, crop and resize operations, you'd be left without the ability to regrid an image onto another (e.g. FA to T1). You'd need to add options to mrregrid to cater for all possible regrid operations (along with all the interpolation options).

Actually, by that stage, I reckon it would be cool to add a few more options to mrregrid to allow transformations during the regridding. It would only add a few options to the help page... :stuck_out_tongue_winking_eye:

draffelt commented 8 years ago

You need a bigger phone to read on your train ride in :) ...as I mentioned:

we can then reserve regridding onto another template image grid (without a transform) for mrregrid.

So yes, mrregrid would also need a -template option to regrid (not transform!) an image onto another grid.

I reckon it would be cool to add a few more options to mrregrid to allow transformations during the regridding.

Yes, yes. I can see your point of view. However, I've also been working on mrtransform a bit lately and feel this is the cleanest solution. It's nice to be part of a democracy. Especially when you are on the winning team :+1:

jdtournier commented 8 years ago

Hehe... Sorry, didn't see you'd already mentioned that. No worries, go for it.

On that note, I notice the Nexus 4 won't be getting Android 6 Marshmallow - maybe it is time to get a bigger screen...

jdtournier commented 8 years ago

OK, now I've got the bigger screen :+1:, what needs to happen with this issue? Can it be closed? Has it been subsumed by other issues...?

draffelt commented 8 years ago

I guess its still open. I vote for having a single mrregrid that resizes, crops and pads. It can also take a -template image for regridding. I also vote for keeping this functionality in mrtransform (i.e. transforming to a template without a -linear transform). I'm really not bothered if there is a tiny bit of overlap in functionality between these two commands. It's easier to keep both from a discoverability point of view, and not really an issue in terms of maintenance (just a single line call to reslice).

As I've said before, I think mrtransform should predominantly be used to for transforming imaging intensities in real space, and adding the extra options for regridding, cropping and padding will just further bloat the options.

thijsdhollander commented 8 years ago

Yep, still fully agree with @draffelt on this as well. I'm not bothered by overlap in functionality at all, the more ways there are to achieve the same thing, the less we will be bothered by users not finding at least any of those ways. Code duplication is minimal, since most of the core functionality is nicely tucked away in the back end, where it can be maintained in just a single spot.

thijsdhollander commented 8 years ago

To put it in another way: the little extra possible time spent on maintenance will pay off big time by a lot of extra free time not spent on user support. And happy users of course, because they won't need support in the first place (so they also gain time).

You'll see, eventually, the whole planet's economy will win; just give it some time. :wink:

maxpietsch commented 5 years ago

I'd like to extend mrcrop and mrpad to also work on axes beyond 3D and use template images as options. I've coded up an mrgrid command that incorporates the existing functionality of these and mrresize and added mrtransform's -template option with identity transformation.

First, there is more than just one line of code duplicated between mrtransform and mrgrid. Not a big deal but not completely comfortable with it either. Below is the current help page. I'd add -nd (or similar) options to the respective operation to allow operating over more than just the spatial axes. Let me know if you'd like to see changes to the structure of the command or have changed your minds and prefer keeping the separate commands.

     mrgrid: part of the MRtrix3 package

SYNOPSIS
     Regrid an image either by resizing (defining the new image resolution,
     voxel size or a scale factor), cropping, padding, or matching the image
     grid of a reference image.

USAGE

     mrgrid [ options ] input operation output

        input        input image to be regridded.

        operation    the oparation to be performed, one of: resize, crop, pad,
                     match.

        output       the output image.

DESCRIPTION

     resize:
     Note that if the image is 4D, then only the first 3 dimensions can be
     resized.

     Also note that if the image is down-sampled, the appropriate smoothing is
     automatically applied using Gaussian smoothing.

     crop:
     Extent of cropping can be determined using either manual setting of axis
     dimensions, or a computed mask image corresponding to the brain.

     If using a mask, a gap of 1 voxel will be left at all 6 edges of the image
     such that trilinear interpolation upon the resulting images is still
     valid.

     This is useful for axially-acquired brain images, where the image size can
     be reduced by a factor of 2 by removing the empty space on either side of
     the brain.

     pad:
      Zero pad an image to increase the FOV.

     match:
      Provide a reference image to match its image grid (voxel size, image size
     and orientation).

Resize options

  -size dims
     define the new image size for the output image. This should be specified
     as a comma-separated list.

  -voxel size
     define the new voxel size for the output image. This can be specified
     either as a single value to be used for all dimensions, or as a
     comma-separated list of the size for each voxel dimension.

  -scale factor
     scale the image resolution by the supplied factor. This can be specified
     either as a single value to be used for all dimensions, or as a
     comma-separated list of scale factors for each dimension.

  -as image
     resize the input image to match the specified template image voxel size.

  -interp method
     set the interpolation method to use when resizing (choices: nearest,
     linear, cubic, sinc. Default: cubic).

Crop options

  -mask image
     crop the input image according to the spatial extent of a mask image

  -as image
     crop the input image if it exceeds the size of the template image grid.

  -axis index start end
     crop the input image in the provided axis. Overrides mask and template
     options.

Pad options

  -as image
     pad the input image to match the specified template image grid.

  -uniform number
     pad the input image by a uniform number of voxels on all sides (in 3D)

  -axis index lower upper
     pad the input image along the provided axis (defined by index). Lower and
     upper define the number of voxels to add to the lower and upper bounds of
     the axis

  -value number
     pad the input image with value instead of zero.

Match options

  -template image
     match the input image grid (voxel spacing, image size and header
     transformation) to that of the template image.

  -interp method
     set the interpolation method to use when reslicing (choices: nearest,
     linear, cubic, sinc. Default: cubic).

  -oversample factor
     set the amount of over-sampling (in the target space) to perform when
     regridding. This is particularly relevant when downsamping a
     high-resolution image to a low-resolution image, to avoid aliasing
     artefacts. This can consist of a single integer, or a comma-separated list
     of 3 integers if different oversampling factors are desired along the
     different axes. Default is determined from ratio of voxel dimensions
     (disabled for nearest-neighbour interpolation).

  -nan
     Use NaN as the out of bounds value (Default: 0.0)

Data type options

  -datatype spec
     specify output image data type. Valid choices are: float32, float32le,
     float32be, float64, float64le, float64be, int64, uint64, int64le,
     uint64le, int64be, uint64be, int32, uint32, int32le, uint32le, int32be,
     uint32be, int16, uint16, int16le, uint16le, int16be, uint16be, cfloat32,
     cfloat32le, cfloat32be, cfloat64, cfloat64le, cfloat64be, int8, uint8,
     bit.
jdtournier commented 5 years ago

Sounds good to me. Only thing that jumps out at me is the overlap between the pad and crop operations - they feel like the same operation: you can crop with negative padding values, and vice-versa. But I can see the value in making the distinction in terms of discoverability, etc.

Also, interesting to go back over the arguments in this debate 3 years on. Personally, I still reckon all this should go into a single mrtransform app, BTW...

Lestropie commented 5 years ago

Also related to #16, but none of the discussion around the interface for these functionalities actually appears there.

thijsdhollander commented 5 years ago

:+1: @maxpietsch I really like the mrgrid you've put together. I'm very happy for this to replace mrcrop, mrpad and mrresize, yet stay clearly separate from mrtransform. Just a few things to add:

Only thing that jumps out at me is the overlap between the pad and crop operations - they feel like the same operation: you can crop with negative padding values, and vice-versa. But I can see the value in making the distinction in terms of discoverability, etc.

It's indeed great to look back at the arguments above... I guess I haven't changed much either though. :stuck_out_tongue: I'm very happy with this kind of "duplication". I still think we're often overlooking most users don't think in an as abstract way as we do. Negative padding or cropping is all good an all, but if they are forced to resort to either of these to essentially do the positive version of the other, we're going to pay the price in time spent on support; but on top of that, the users also lose that time (and possibly their affinity with the software; we don't want to push them away, right?). That said, it's not a bad idea to allow for negative parameters to both of these options (cropping/padding); I can see that coming in handy for certain kinds of scripts for sure. But I still find it a bit silly to try and weed out as much "duplication" in functionality as possible. Note that a lot of the cropping functionality can "in principle" be achieved via mrconvert -coord (and I use both interfaces interchangeably myself, depending on the context or purpose). But my personal experience with several different users over here has been that nobody naturally thought of resorting to mrconvert for this; hence there's a 100% valid raison d'être for an explicit cropping functionality to exist. Same argument goes for padding, as explicitly named and listed in addition to cropping.

Also, interesting to go back over the arguments in this debate 3 years on. Personally, I still reckon all this should go into a single mrtransform app, BTW...

I've over these years, and especially in the most recent one, become more convinced of the opposite.. :grin: :man_shrugging: It's a real struggle to try and explain people, particularly with an FSL background, the concept of our scanner/world/... space; some of them (otherwise very clever people!) almost stubbornly seem to keep talking in terms of "DWI space" and "T1 space" and "modality-of-choice space", even after all the required motion correction has been done, but simply because essentially the grids are not the same. The separation between mrgrid kind of functionalities (i.e. mrcrop, mrpadand mrresize up to now) and mrtransform is one of the best means of making this clear to them: the one simply manipulates the grid, but the actually "object" in the image remains in the same position in space, while the other effectively transforms that "object" in the scanner/world/.. space. Allowing for the -template functionality (with identity transform) in mrgrid makes this clean separation even better (I've, for example, experienced it being annoying to explain all of the above to those users, but then have to tell to use mrtransform -template to simply regrid without transform). So again, very happy with the proposal. I'd still allow the existing mrtransform -template even in the absence of a transform (and hence using identity transform) though. Again, this approach simply caters for more users on average; i.e. "discoverability".

@maxpietsch I do see an opportunity here to do away with another possible confusion in terminology, which is the "resizing" type of terminology: I still very often get from users that they (initially) believed this to be a transformation kind of resizing, i.e. a rigid, or even anisotropic (affine) type of scaling of the "object" in scanner space. Looking at the interface you've put together, I think we can fix this simply by a different word choice: both the resize terminology as well as all related options in both the "resize options" as well as "match options" categories could be replaced (terminology wise) by "regridding". That defines them both quite well (and essentially communicates that interpolation can happen), but also sets them jointly well apart from the cropping and padding. Everything combined (regridding / cropping / padding) still all acts on the grid, so mrgrid as a command name remains entirely justified (and because it also allows to crop/pad, we're not calling it e.g. "mrregrid"). Just as an example of how (I think) think might sound, this is how the synopsis might then read:

Instead of

Regrid an image either by resizing (defining the new image resolution, voxel size or a scale factor), cropping, padding, or matching the image grid of a reference image.

we could then have

Modify the grid of an image either by regridding (defining the new image resolution, voxel size, a scale factor or match the grid of a reference image), cropping or padding.

So I'm essentially:

The matching options can then also move into the currently named "resizing options" category, but the category would be renamed to "regridding options". This makes sense, as e.g. in your example there's already duplication of the -interp option in both categories, and if I'm not mistaken the -oversample option also applies to several other "regridding options", such as when -voxel defines a larger voxel size compared to the input image.

What do you reckon; makes sense? Or am I getting something wrong?

maxpietsch commented 5 years ago

Ok, I had a discussions about this with 3 people and got 3 opinions...

The differences between 'transforming', 'regridding' and 'modifying the grid' might be confusing. I think the main concepts are: is there a change in location in real world coordinates and is the image interpolated?

in mrtransform currently,

in mrgrid

The main distinction is that real world coordinates are supposed to be changed via mrtransform, not via mrgrid.

The downside of having mrgrid not integrated into mrtransform is that the target reference image needs to be created upfront. Changing the voxel size of an image while transforming is not possible but would require resizing the template image before applying the transformation.

The downside of integrating everything into mrtransform is that it already is a monster of a command. Also, I guess that users would expect operations to be applied in the order as specified on the command line.

mrtransform in.mif -linear affine -template reference.mif -voxel 1.0 -pad_axis 0 10 10 output.mif

should yield a different output compared to

mrtransform in.mif -pad_axis 0 10 10 -linear affine -template reference.mif -voxel 1.0 output.mif

or

mrtransform in.mif -template reference.mif -voxel 1.0 -pad_axis 0 10 10  -linear affine output.mif

and so on. If we allow this, there would be quite a lot of scope for unexpected results. If we do not want to allow that, then we would need to define an order of operations. For instance transform first, regrid after -- expcept for warps where we apply regrid options to the warp (or its -template space), then apply the warp. The user needs to be aware of this, and we'd need to make changes to the warp handling.

I'd lean towards that this is too complex for too little gain.

thijsdhollander commented 5 years ago

Ok, I had a discussions about this with 3 people and got 3 opinions...

I'm not surprised :grin: . That's probably the main reason to be careful about this, and make sure we define and/or separate things well and clearly.

I think the main concepts are: is there a change in location in real world coordinates and is the image interpolated?

Agreed, and I'd say: also in that order. The main distinction, I reckon, is whether a transformation happens to the object or not (i.e. whether there's the possibility to change the location in real world coordinates). That's what I'd genuinely call "a transformation", hence a responsibility/functionality mrtransform offers.

The interpolation (or not) is secondary to that, I would argue. It's something that happens "when necessary", i.e. it doesn't have to happen when you transform rigidly (as that can be achieved by modifying the header only), but for most other transforms (affine/non-rigid) it'll almost always have to happen obviously. Same thing in the context of mrgrid: certain changes to the grid don't require interpolation (e.g. cropping), but others will (e.g. changing the resolution). To put it differently: you could see interpolation as being merely a "necessary evil" of the fact that we work with discrete voxels.

The main distinction is that real world coordinates are supposed to be changed via mrtransform, not via mrgrid.

In line with the above: fully agreed.

The downside of having mrgrid not integrated into mrtransform is that the target reference image needs to be created upfront. Changing the voxel size of an image while transforming is not possible but would require resizing the template image before applying the transformation.

I personally don't see that as a downside; it makes full sense to me even. If you need to change the voxel size together with the transformation (e.g. to avoid double interpolation), you can create a grid up front as a template (and feed that to the -template option of mrtransform), or, in case of a warp, you can simply resample/interpolate the warp itself. The latter, in my experience, is the common solution (for warps); and allows even manual control over how the warp has to be interpolated (different choices have different interpretations and implications here).

But I also think it's, for most users, a less common scenario. And if it's a scenario they encounter, it's often due to the result needing to use the same voxel grid of an existing target image (which allows certain things in a more straight forward way down the track for some pipelines), which can simply be fed to -template.

The downside of integrating everything into mrtransform is that it already is a monster of a command.

Yes, exactly. Your examples are just some of the many scenarios of where that becomes a nightmare to define, implement, document, support (as many users will have trouble understanding some of the intricacies of that), and ultimately also maintain (so as to not break the intricate logic of the order of those steps). And not that I personally care much about the Unix philosophy, but others here do, I thought... :wink: "small", "modular", not "large", "monolithic"

I'd lean towards that this is too complex for too little gain.

Couldn't agree more.

So apart from the clear mrtransform vs mrgrid split; see also some of my comments above on how I think we might further create clear structure within mrgrid itself; based on that second criterion we identified: interpolation (defining the new image resolution, voxel size, a scale factor or match the grid of a reference image) or not (cropping or padding).

maxpietsch commented 5 years ago

Ok, also implemented the posibility of regridding to template image but with changed voxel size. I am happy with the current layout of the command, let me know if you have objections before I wrap it up:

     mrgrid: part of the MRtrix3 package

SYNOPSIS
     Modify the grid of an image either by regridding (defining the new image
     resolution or match the grid of a reference image), cropping or padding.

USAGE

     mrgrid [ options ] input operation output

        input        input image to be regridded.

        operation    the oparation to be performed, one of: regrid, crop, pad.

        output       the output image.

DESCRIPTION

     regrid:

     Operations that change the voxel grid and require interpolation of the
     image such as changing the resolution or location and orientation of the
     voxel grid. Note that the image content remains in place in real world
     coordinates. Only the resolution of the first 3 dimensions can be changed.
     If the image is down-sampled, the appropriate smoothing is automatically
     applied using Gaussian smoothing unless nearest neighbour interpolation is
     selected or oversample is changed explicitly.

     crop:

     Extent of cropping can be determined using either manual setting of axis
     dimensions, or a computed mask image or via a reference image. If using a
     mask, a gap of 1 voxel will be left at all edges of the image such that
     trilinear interpolation upon the resulting images is still valid. This is
     useful for axially-acquired brain images, where the image size can be
     reduced by a factor of 2 by removing the empty space on either side of the
     brain.

     pad:

     Pad an image to increase the FOV. Allows mixtures of negative and positive
     values to pad and crop the image simultaneously.

Regridding options (involves image interpolation, applied to spatial axes only)

  -template image
     match the input image grid (voxel spacing, image size, header
     transformation) to that of a reference image. The image resolution
     relative to the template image can be changed with one of -size, -voxel,
     -scale.

  -size dims
     define the size (number of voxels) in each spatial dimension for the
     output image. This should be specified as a comma-separated list.

  -voxel size
     define the new voxel size for the output image. This can be specified
     either as a single value to be used for all dimensions, or as a
     comma-separated list of the size for each voxel dimension.

  -scale factor
     scale the image resolution by the supplied factor. This can be specified
     either as a single value to be used for all dimensions, or as a
     comma-separated list of scale factors for each dimension.

  -interp method
     set the interpolation method to use when reslicing (choices: nearest,
     linear, cubic, sinc. Default: cubic).

  -oversample factor
     set the amount of over-sampling (in the target space) to perform when
     regridding. This is particularly relevant when downsamping a
     high-resolution image to a low-resolution image, to avoid aliasing
     artefacts. This can consist of a single integer, or a comma-separated list
     of 3 integers if different oversampling factors are desired along the
     different axes. Default is determined from ratio of voxel dimensions
     (disabled for nearest-neighbour interpolation).

  -value number
     Use number as the out of bounds value (Default: 0.0)

  -nan
     Convenience option for -value NAN.

Crop options (no image interpolation is performed, header transformation is adjusted)

  -mask image
     crop the input image according to the spatial extent of a mask image

  -as image
     crop the input image if it exceeds the size of the template image grid.

  -axis index start end
     crop the input image in the provided axis. Overrides mask and template
     options.

  -nd
     Crop all, not just spatial axes.

Pad options (no image interpolation is performed, header transformation is adjusted)

  -as image
     pad the input image to match the specified template image grid.

  -uniform number
     pad the input image by a uniform number of voxels on all sides

  -axis index lower upper
     pad the input image along the provided axis (defined by index). Lower and
     upper define the number of voxels to add to the lower and upper bounds of
     the axis

  -value number
     pad the input image with value instead of zero.

  -nd
     Pad all, not just spatial axes.

Stride options

  -strides spec
     specify the strides of the output data in memory; either as a
     comma-separated list of (signed) integers, or as a template image from
     which the strides shall be extracted and used. The actual strides produced
     will depend on whether the output image format can support it.

Data type options

  -datatype spec
     specify output image data type. Valid choices are: float32, float32le,
     float32be, float64, float64le, float64be, int64, uint64, int64le,
     uint64le, int64be, uint64be, int32, uint32, int32le, uint32le, int32be,
     uint32be, int16, uint16, int16le, uint16le, int16be, uint16be, cfloat32,
     cfloat32le, cfloat32be, cfloat64, cfloat64le, cfloat64be, int8, uint8,
     bit.
jdtournier commented 5 years ago

Also, I guess that users would expect operations to be applied in the order as specified on the command line.

I think this is probably the most compelling reason to keep them separate. There's no simple, clean way to manage this that doesn't make things more difficult to reason about...

also implemented the posibility of regridding to template image but with changed voxel size

So this would be applied within mrtransform, presumably? That would help as well - but strictly speaking, given the way the logic is set up, that should be an mrgrid job, right? No harm in having it though...

Otherwise, I note you have a duplicate of the -value option, and no equivalent -nan option for pad. Maybe these should be part of a separate option group, specifying what happens to out of bounds values - however they may arise?

I also note other options are duplicated: -axis, -nd, -as. Does that not cause issues with the cmdline parsing backend...? Maybe it's fine, it's been a while since I cooked this up... But as before, I'd favour merging these into a single option group that applies to both. The only options that aren't common to both currently are -mask (crop only), and -uniform (pad only). I reckon -uniform might be handy for crop too? But given the near-complete overlap, why not have a single option group for both pad & crop? I admit there's not much point in -mask for pad, or -value for crop, but there's no harm in having them there, and labelled as applicable only in those cases...?

Finally, it seems to me that the -template and -as options are essentially the same concept. Why not use a single -template option to cover all these cases, since it would then be common to all operations?

So the above would lead me to have this overall layout:



As mentioned above, there is a potential difference in interpretation in the -axis option, with pad presumably padding with that many voxels before & after, whereas crop presumably picks the coordinates directly? I assume this means there is no simple way to crop by e.g. 3 voxels from both sides...? Which is the most likely use case? I would have thought that cropping by some number of voxels was probably likely to be the most useful, but I'm not sure. Otherwise, cropping to specified coordinates can already be achieved in mrconvert, but I think it would be odd not to cater for that use case here too...

Before going onto that though: I assume the command allows for multiple (non-conflicting) -axis options...?

I'm not sure how to handle this in the ideal case, but I think something using number sequences might work well, if we use a convention like:

This then opens up the option of having negative values (not sure how keen you guys are going to be on the idea...):

At which point we've effectively merged the two pad & crop operations into one... So in this case, we would treat a range (colon-separated) identically for both crop & pad, but treat a single or comma-separated list of two numbers as relative, with the sign of the numbers essentially inverted in one case vs the other.


Other thoughts:

Sorry, lots of random thoughts in here, just thought I'd air them here for discussion...

maxpietsch commented 5 years ago

The saga continues, or so people say. TL/DR see bottom.

Interface:

Having the out of bounds value in a general option group makes sense. However, currently it does not apply to cropping as crop does not allow negative values. Logically I'd expect common options to apply to all operations but happy to change it.

I used -template and -as because the former triggers regridding with interpolation to that image (as in mrtransform), the latter just uses the image to set the grid extent or dimensionality without interpolation. The -as terminology is inspired by torch's reshape_as. I feel that using the same option names for both would muddy the interpolation distinction a bit.

there is a potential difference in interpretation in the -axis option, with pad presumably padding with that many voxels before & after, whereas crop presumably picks the coordinates directly?

Yes, and I kept it that way for backwards compatibility. Having a mixture of relative and absolute axis indices might be confusing to new users but changing this behaviour might stump old users. Not sure what is better.

I like the range (colon-separated) vs signed single or comma-separated tuple idea but not sure if we can handle the edge case 2:3 vs 2,3? If we go for -pad -crop instead of pad -axis, or crop -axis what should happen with -pad 0 10 -crop 0 3:100? Also, we would need to make it clear to existing users that the behaviour has changed. Maybe deprecate mrpad and mrcrop and replace them with scripts that translate the axis options to the correct mrgrid format. If we get rid of pad and crop operations, I'd make-as to two options crop_as and pad_as to allow padding independently of cropping.

So this would be applied within mrtransform, presumably? That would help as well - but strictly speaking, given the way the logic is set up, that should be an mrgrid job, right? No harm in having it though...

It's implemented in mrgrid. I was not planning to implement this in mrtransform as it is a regrid job but I guess there is no harm in duplicating code? ;)

Otherwise, I note you have a duplicate of the -value option, and no equivalent -nan option for pad.

Yes, I added the -value option. Was tempted to get rid of -nan but kept it for backward compatability. Happy to make one default group and put both there as you suggested.

how does the (current) -as option handle the case where the template is not aligned with the input image?

It ignores the header transformation, as it is applied to the grid only. Hence the grouping into regrid and pad/crop arguments.

we could ditch the operation argument altogether, and 'regrid' to the -template ... with some ambiguity regarding whether the scaling is applied before or after the padding/cropping...

Yes, the ambiguity is the problem.

mrgrid input.mif -template other.nii -pad 0 10 -crop 1 3:100 -scale 2 output.mif

can already unambiguously be achieved (using a single interpolation) via mrgrid input.mif regrid -template other.nii -scale 2 - | mrgrid - reshape_or_similar -pad 0 10 -crop 1 3:100 output.mif or via mrgrid input.mif reshape_or_similar -pad 0 10 -crop 1 3:100 - | mrgrid - regrid -template other.nii -scale 2 output.mif depending on what you meant ;)

Code:

I kept the code paths for crop and pad separate as cropping relies on the subset adapter which can not handle out of bounds values. If we go for the 1,2 tuple vs colon syntax or if we just translate cropping to negative padding, we could use the padding code path for everything. That meant we'd need to modify the -mask option anyway so could implement this for padding as well. Uniform cropping can already be achieved via the pad argument and negative extent.

I assume the command allows for multiple (non-conflicting) -axis options...? yes, and there is no difference between repetitions, hence pad and crop need to be mutually exclusive.

How about:

mrgrid in operation out

operation: one of regrid, reshape_or_better_name

main (default) option group:
   -fill value
   -nan

regrid option group:
   -template (if set, -size -voxel or -scale are applied to tempalte grid)
   -size dims
   -voxel size
   -scale factor
   -interp method
   -oversample factor

reshape_or_better_name option group:
   -select index start end (equivalent to mrconvert -coord, -pad and -crop applied after select)
   -pad index lower upper (pad by n voxels, apply to all axes with -pad : lower upper)
   -pad_as image
   -crop index lower upper (crop by n voxels, apply to all axes with -crop : lower upper)
   -crop_as image
   -crop_mask image
   -nd (-<operation> : X X and <operation>_as options are applied to non-spatial axes as well)
jdtournier commented 5 years ago

The saga continues, or so people say. TL/DR see bottom.

🤣 Indeed... and it's not over - although bear in mind these are just general deliberations / internal musings / thinking aloud.

Having the out of bounds value in a general option group makes sense. However, currently it does not apply to cropping as crop does not allow negative values. Logically I'd expect common options to apply to all operations but happy to change it.

I'm not that worried about it. As long as the effect of the options is clear, the fact that they're not relevant in all contexts doesn't bother me. For the -value (-fill) option, of course it's not used in a crop context, but given the clear parallel with pad, it's still clear what it means.

I used -template and -as because the former triggers regridding with interpolation to that image (as in mrtransform), the latter just uses the image to set the grid extent or dimensionality without interpolation. The -as terminology is inspired by torch's reshape_as. I feel that using the same option names for both would muddy the interpolation distinction a bit.

I don't see the need to make that distinction - the operation to be performed already tells you what you expect to happen. If anything, I feel having two distinct labels attached to what is really the same concept (this is the 'reference grid') actually confuses things. Assuming we stick with the single mandatory operation argument (likely, see below), then I think we should use the same label everywhere. I don't feel all that strongly about which one we use, there are different good options here: -template, -as, or even -reference. I do think -template is preferable since it matches the term currently used in mrtransform, but I don't really mind (I quite like -as, actually...). But the main point is, I think there should be one option to set the reference grid, and the operation specified uniquely determines what happens with it.

there is a potential difference in interpretation in the -axis option, with pad presumably padding with that many voxels before & after, whereas crop presumably picks the coordinates directly?

Yes, and I kept it that way for backwards compatibility. Having a mixture of relative and absolute axis indices might be confusing to new users but changing this behaviour might stump old users. Not sure what is better.

OK, given that this is a new command, taking over functionality previously in other commands, I don't see the rationale for trying to maintain backwards compatibility. This is basically why I came up with all these suggestions in my last post. We have an opportunity to rethink the interface, so let's not be constrained by the previous commands - but let's also make sure it remains vaguely coherent with the rest of the software.

I like the range (colon-separated) vs signed single or comma-separated tuple idea but not sure if we can handle the edge case 2:3 vs 2,3?

What I was thinking in this case is that we can't use the standard number sequence handler (parse_ints()): it's too broad, and allows for increments (e.g. 0:3:end), going backwards (end:0), etc - things we don't necessarily want to allow here. So we'd have explicitly handling, splitting by a single comma or colon, and acting accordingly. In this context, 2:3 selects coordinates, 2,3 pads/crops by those numbers - no ambiguity.

If we go for -pad -crop instead of pad -axis, or crop -axis what should happen with -pad 0 10 -crop 0 3:100?

Two options:

But this is only if there's appetite for moving from an interface with a single mandatory operation, to potentially multiple operations as options. It's just an idea, and as you mention, a combination of commands can already be used to achieve all of the relevant use cases with better clarity. I don't like the redundant data copy involved in that, but it's not exactly a massive problem either...

So this would be applied within mrtransform, presumably? That would help as well - but strictly speaking, given the way the logic is set up, that should be an mrgrid job, right? No harm in having it though...

It's implemented in mrgrid. I was not planning to implement this in mrtransform as it is a regrid job but I guess there is no harm in duplicating code? ;)

OK, got wrong end of stick. Ignore me.

how does the (current) -as option handle the case where the template is not aligned with the input image?

It ignores the header transformation, as it is applied to the grid only. Hence the grouping into regrid and pad/crop arguments.

OK, in which case I'm confused (happens a lot these days...). Does this mean that if I have images A & B with mask M all on the same grid, then mrgrid A crop -mask M C, so that cropping happens on all sides, then try to mrgrid B crop -as C D, there is no guarantee that C & D will line up in real space...? Does it essentially only trim the ends of the axes to match the -as image?

Conversely, if I have images A & B not on the same grid, there is no guarantee that mrgrid A crop -as B C will produce an image C that encompasses the space spanned by B - it might even not overlap with it at all in real space?

I think the only way to handle this option in a predictable manner is to require the voxel grid to already match to good precision, and then apply whatever padding / cropping is necessary to make the output grid match that in the -template / -as image more or less voxel-wise.

Or am I reading this wrong - again...?

we could ditch the operation argument altogether, and 'regrid' to the -template ... with some ambiguity regarding whether the scaling is applied before or after the padding/cropping...

Yes, the ambiguity is the problem.

OK, I think I agree sticking with the single mandatory operation argument is preferable in the interest of clarity - happy to stick with that.

I kept the code paths for crop and pad separate as cropping relies on the subset adapter which can not handle out of bounds values.

I reckon it might be feasible to modify the subset adapter to support out of bounds conditions? Shouldn't™ be too hard, right?

If we go for the 1,2 tuple vs colon syntax or if we just translate cropping to negative padding, we could use the padding code path for everything.

Yes, that's what I had in mind. But I don't think the interface needs to therefore offer a single crop/pad operation. We can offer both, and simply interpret the arguments differently depending on context. So this would mean that crop -axis 0 2,-3 is the same as pad -axis 0 -2,3. In practice, this is a very rare use case anyway, users will undoubtedly always use one or the other...

That meant we'd need to modify the -mask option anyway so could implement this for padding as well. Uniform cropping can already be achieved via the pad argument and negative extent.

Cool. But would be good to make the interface accept any of these combinations without the user having to work out which combo works and which doesn't... It also makes our lives easier if we can reduce it to a single code-path.


So on that basis, how about:

mrgrid in operation out

operation: one of regrid, crop, pad

main (default) option group:
   -template reference
   -fill value
   -nan

regrid option group:
   -size dims
   -voxel size
   -scale factor
   -interp method
   -oversample factor

crop and pad option group:
   -axis indices spec (with clarification in description / examples)
   -mask image
   -nd (-<operation> : X X and 

What do you all reckon...?

maxpietsch commented 5 years ago

🍾 28 Sep 2015 (#364) - 7 May 2019 (#1609) That's 25 dog years 🐶