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
293 stars 180 forks source link

mrtransform: add -flipx option to handle flirt transforms #108

Closed jdtournier closed 10 years ago

jdtournier commented 10 years ago

This is needed to account for the different coordinate system used by flirt - see discussion on Google+ for details.

jdtournier commented 10 years ago

Actually, I think the cleanest solution to this is to provide a simple python script to convert between the flirt & MRtrix transform conventions. That would leave the mrtransform as simple as it can be, and make it very explicit why the conversion is necessary - in the previous version, there is nothing to indicate that you need to add a -reference image and the -flipx option to apply a flirt transform (aside from the FAQ that is).

jdtournier commented 10 years ago

OK, quick update: this is proving harder to handle than I'd anticipated. Main problem being what happens when you have non-standard strides - all works fine when you have strides 1,2,3...

What I'll probably do is push what I've got so far, with the warning that it'll only work for standard strides. It's a new app to convert the flirt transform to one that MRtrix can use. Hopefully I'll have the non-standard strides case sussed out soon enough...

Lestropie commented 10 years ago

Beginning to think that this may just be one more band-aid on top of what is a more fundamental problem. You've had a lot more experience with this stuff than I have, I'd love to know exactly what's going on with the incompatibilities between MRtrix and FSL. Is it a NiFTI ambiguity thing? Do we need to internally force the strides to +1,+2,+3 and adjust both the qform and sform transforms accordingly whenever we write to NiFTI? Or is there a RAS v.s. LAS thing going on? Or is FSL genuinely interpreting things incorrectly even by the NiFTI standard?

I know there's a couple of FSL commands that have their own internal quirks and need to be dealt with accordingly, but it seems as though there's a bit of a stand-off where each side is blaming the other side's software for not interpreting the image data correctly (and not just in terms of image v.s. scanner coordinates). All seems far too difficult for something that should have been thoroughly standardized a long time ago.

jdtournier commented 10 years ago

I feel your frustration...

I agree it's a pain to deal with these different conventions, but as far as I can tell, everything is as it should be. FSL does interpret the data correctly (it knows which way is AP, etc), but doesn't necessarily do anything about it. Leads to issues displaying images in fslview where they appear on different panes in different orientations, but the orientation labels are correct - more of a problem when trying to overlay an image with a different orientation that hasn't been reformatted to fit the first image (this has come up a few times on the mailing list). In MRtrix, we (as in I) chose to treat the same image stored using a different combination of strides and orientation as the same, and I stand by that decision (we also had a lengthy chat about that if you recall - issue 74 on GitLab). We could have used the 'standard' convention (basically FSL's), but I for one would never have been happy with that. The other problems stem from the fact that MRtrix supports a wider subset of the NIfTI data types than does FSL (notably bitwise), and that can also lead to frustration.

As to the issue at hand, again there is no ambiguity: FSL documents how they represent the transformation matrix on the flirt FAQ. On that front they're free to do what they want - there is no standard coordinate system that must be used to represent an affine transform. They chose to store the matrix mapping from the axes of one image onto the axes of the other. This is fine, but only works for the images that were used in the registration - you can't apply that transformation directly on a different image without specifying the transforms of both images that it refers to. We chose to use a matrix that maps from & to real/scanner coordinates, which makes it applicable to any image directly. So there is some housekeeping to do to convert between the two, especially when the coordinate system flirt uses may also be flipped left/right depending on whether it form a left-handed coordinate system or not... And the fact that the order of the axes of the images is dependent on the strides, etc, which adds a little spice into the mix... (what I'm struggling with right now)

Basically, a lot of these 'incompatibilities' stem from my decision to do everything in real/scanner space wherever possible - and as it turns out we use the exact same coordinate system as NIfTI. Most other software packages use the image's own axes as their coordinate system (this is again the basic problem converting between MRtrix & FSL DW schemes). From everything I've seen so far, the minor niggles we have converting between the two are far outweighed by the convenience of being able to mix & match different types of images, both during processing and on the viewer.

So while I agree it's frustrating, as far as I can tell everything is as it should be: well-defined. It is annoying to have to convert between them, but at least you have the information to do so - way better than the bad old days of Analyse images, where you simply couldn't tell right from left...

jdtournier commented 10 years ago

As to the issue of handling of the strides when writing out to NIfTI, I've spent a fair bit of time on this (issues #95 & #64) to make sure the strides on an input NIfTI are preserved on write-out whenever possible, which I think addresses a few of the issues people were having (notably with the overlay in FSLView). Internally, they undergo the same treatment as any other image: whichever axis is closest to left-right appear as the first axis, whichever is closest to anterior-posterior appears as the second axis, etc - this is done by adjusting the transform matrix and strides accordingly. But at least an image and its reformatted equivalent end up being treated identically everywhere...

jdtournier commented 10 years ago

OK, just pushed a new command called mtxedit to master - stands for 'matrix edit' - alternative name suggestions welcome. The idea behind this name was that the command would eventually allow relatively versatile matrix operations, to allow multiplication, inversion, etc, and capable of operating on matrices of any size (not just 4x4 transforms). Eventually the flirt conversion operation would be an option rather than the default mode of operation...

Alternative name might be mtxcalc...?

Lestropie commented 10 years ago

I suspect that if that's what flirt is doing, you won't be able to solve the problem with a generic matrix editing command; you'll need a dedicated script / command that reads the source & target images fed to flirt, and figures out the real-space affine transformation taking place based on the two image header transforms and their strides (and possibly also a flag to say it's in LAS space).

Got no qualms with the benefits of scanner space. But it sounds as though the problem lies in the definition of the axis directions in the NiFTI header, which FSL is subsequently ignoring. I'm guessing this was the original rationale for the axis-reshuffling-on-image-load-to-approximately-axial-orientation concept? If the fundamental issue is at the FSL end, then we've probably got no choice but to keep doing the -stride +1,+2,+3 trick in the scripts, and try to provide support for these edge cases (or, better yet, provide alternatives within MRtrix).

BTW, personally prefer matrixcalc to mtxedit; many basic operations could also be covered using mrcalc if #8 is implemented (obviously multiplication / inversion are a different kettle of fish).

jdtournier commented 10 years ago

I suspect that if that's what flirt is doing, you won't be able to solve the problem with a generic matrix editing command; you'll need a dedicated script / command that reads the source & target images fed to flirt, and figures out the real-space affine transformation taking place based on the two image header transforms and their strides (and possibly also a flag to say it's in LAS space).

That's exactly what this new command does - it's too complicated for a script. And we have all the information available to handle the non-standard strides case, it's just a matter of figuring out exactly what sequence of operations to perform.

There is also no need for a flag for LAS space - NIfTI is actually well defined on that front. It's not a particularly obvious or simple specification (particularly the choice of two different transforms with no indication of which is 'correct'), but there is no ambiguity otherwise. You're probably thinking about the issues with its previous incarnation as the Analyse format, which did end up being ambiguous due to different software packages interpreting it differently (understandably so - I've read the Analyse specification, and nowhere does it say which direction the axes should point...).

I'll change the name to matrixcalc - I agree it's much more obvious than mtxedit...

Lestropie commented 10 years ago

Cool and cool; wasn't thinking of Analyze, was under the impression that NiFTI and MRtrix are RAS but FSL was LAS, and therefore that could cause issues if things are defined in terms of image position but the command doesn't know what software created the input data (e.g. in mesh2pve I needed an explicit -first flag to interpret the .vtk's from FIRST correctly)

But I think it's going to be tough incorporating this process into a command that is designed to perform generic operations on matrix data in a similar manner to mrcalc. I think it needs its own command (which it currently is) with a more appropriate name, and matrixcalc is a separate project.

jdtournier commented 10 years ago

Well, what I had in mind was that eventually the syntax would become similar to mrcalc (reverse polish), and the flirt conversion would become an option like this:

 $ matrixcalc transform.txt -flirt in.nii ref.nii output_transform.txt

The current syntax is identical apart from the -flirt bit. This would then leave the door open for doing things like composing this modified transformation with other MRtrix-compatible matrices, or inverting prior to use, etc. e.g.:

  $ matrixcalc transform.txt -flirt in.nii ref.nii -invert other_transform.txt -mult output_transform.txt

The only odd thing about it would be that the -flirt option would look ahead for its arguments, in contrast to the other operations. Not too fussed about this though, the same issue already exists in mrcalc to handle the -nthreads option...

jdtournier commented 10 years ago

Other idea would be to accept image names as matrices, in which case the command would use its transform, and maintain a link to the image header. Would be useful for things like flipping along an axis (which is needed to handle the flirt matrix), where you need to know the size of the image along that axis as well as the transform matrix in order to shift the origin appropriately.

In this case, the flirt conversion option could also use the reverse polish notation, so the command would look like:

  $ matrixcalc transform.txt in.nii ref.nii -flirt output_transform.txt
Lestropie commented 10 years ago

It might work, I'm not completely against it having learned from previous encounters (I actually quite like tckedit now). But it depends on whether it feels clunky mashing it into matrixcalc, or would be cleaner and more 'discoverable' (your words :-P ) as its own command. If you were to split matrixcalc and have a separate command transformcalc specifically for handling 4x4 transforms, it might fit better in there - handling images as inputs would make more sense then too.

But also right now, at this very point in time, when all the command does is handle flirt transform weirdness, matrixcalc seems an odd choice; that's describing future behaviour.

jdtournier commented 10 years ago

Agree. But if people start using it now, I don't want their scripts to break if we decide to move the functionality elsewhere, which is why I'd rather settle on the final name now.

As to discoverability, yes, that had crossed my mind too... For the flirt conversion, I don't think either name would alert people to the availability of this option. This would have to be spelt out clearly in the relevant FAQ or wiki entry somewhere (as it is currently for mrtransform). As to using the name transformcalc, I have two issues with it: one is that transformcalc might (in future) appear to also operate on non-linear transforms, which it clearly wouldn't. I think most people will appreciate that their transforms are matrices, and that matrixcalc is therefore an appropriate command for that. Second issue is that limiting the command to only operate on 4x4 matrices seems a shame, when most of the code would be identical were it to allow operating on arbitrary matrices.

Lestropie commented 10 years ago

Well, command name changes in scripts are pretty easy to identify and update. But yeah, 'transform' ambiguity between image header transform / affine registration transform / non-linear transform is tough (also need to make sure anything that should perhaps actually be in mrtransform goes there)... I guess the only other thing is confirming that all matrix operations would in fact be identical to the generic 4x4 case? (e.g. wouldn't using some generic matrix operations result in a non-[0 0 0 1] lower row? Or improper interpretation of the rightmost column as offsets?) But if that all behaves as it should, then let's leave it as matrixcalc (perhaps incorporating the -flirt option syntax, even if only to explain its use in the help page and the fact that the main command functionality isn't yet implemented) and see how it evolves.

jdtournier commented 10 years ago

I guess the only other thing is confirming that all matrix operations would in fact be identical to the generic 4x4 case? (e.g. wouldn't using some generic matrix operations result in a non-[0 0 0 1] lower row? Or improper interpretation of the rightmost column as offsets?)

That's my only concern. It is theoretically identical - for instance, the flirt conversion doesn't treat the bottom row any differently - but there will almost certainly be rounding errors introduced at various stages... We could add one more option to clean it up as a final step if that proves to be a problem.

jdtournier commented 10 years ago

So it turns out that matrixcalc works fine as-is to convert flirt transforms regardless of the strides in the images fed to flirt... No idea why my testing wasn't working yesterday, I was tearing my hair out trying to figure out why... Oh well.

Guess I'll close this for now then.

draffelt commented 10 years ago

This whole FSL issue will be less of a problem once we have a robust affine registration.

I like the idea of having two separate commands.

matrixcalc strictly for matrix only operations (ie, no image dimensions required). This could deal with both text or image based matrices in reverse polish. You could even save the output matrix in an existing image, requiring the -force option to override.

transformedit for all spatial transformation issues. FSL conversions, ANTS conversions (warps included), flipping, inverting warps, composing multiple transforms (warps included) etc. Also maybe even some options to generate affine transforms with used defined rotations/shear/scale.

jdtournier commented 10 years ago

Yes, I'd love to see registration in MRtrix... :)

(and yes, I'm sure you'd like an ROI tool too...)

Seems the people want two commands. Guess we'll have to go along with that. Fair enough if you intend for the transformedit (transformcalc?) command to also handle non-linear warps (didn't you have a warpedit command for that...?).

I'll rename it (again) when I get in. Might also change the syntax straight away to what we'll eventually use. Are we planning reverse polish for this one too? I guess it's not required for this since the operations will be sequential - no possibility of a branch (i.e brackets). Am I right in assuming this? When editing transforms, there will never be any operations like (a op b) op (c op d), etc? Assuming that's OK, I'll just assume we're happy to apply each operation in succession as it appears on the command line...

draffelt commented 10 years ago

I think transformedit is more appropriate if we are using it for a mix bag of different tasks. I think using 'calc' strictly for those commands that use reversepolish makes sense (mrcalc and matrixcalc). However, I'm not too fussed either way,

Agreed, I don't think reverse polish is needed for transformcalc(edit).