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
290 stars 179 forks source link

FSL bvecs/bvals handling #51

Closed jdtournier closed 10 years ago

jdtournier commented 10 years ago

Current implementation will search for files called bvecs/bvals co-located with the DWI data, and use them if found. Otherwise, it'll look for files with the same prefix as the DWI image and the _bvecs/_bvals suffix. This is fine, but there is actually no set convention as to what these files are called. This means there is no way to supply an FSL encoding if the name doesn't match that expected. Maybe we should add an additional option like -fslgrad to supply both filenames, which would get around the problem. It would also allow us to remove the autodetection steps currently taken to try to discover these encoding files, which would simplify the code somewhat and reduce the chances of any unexpected side effects...

dchristiaens commented 10 years ago

+1 for a separate option. I'm also not a big fan of auto-detecting files with similar prefixes (I've had issues before where some packages use "bvecs" and others "bvec"), although that's mostly a matter of documentation. In fact, wouldn't it be possible to use the current -grad option, and detect if one or two arguments are specified?

One concern though, I think FSL inverts the x-axis and specifies the gradient directions w.r.t. the image coordinate system. It may get a little confusing if all commands support two different conventions for the gradient encoding. Wouldn't if be more clear to deal with this conversion in one place at the start of the pipeline (e.g. mrconvert)?

jdtournier commented 10 years ago

You can't have an option with a variable number of arguments - it's impossible to tell whether the additional argument is supposed to be for the option or if it's supposed to be a full-blown argument for the command. This is why a second -fslgrad option would be needed.

As to the handling of the different coordinate systems, MRtrix already handles this internally without any problem. The only requirement is that the image supplied matches its encoding file. Having a dedicated -fslgrad option would I think be explicit enough. And there would be nothing stopping you from running mrconvert with that option, which would include the bvecs/bvals into the output image's header - an option that I agree is cleaner, but since the infrastructure is already in place for more generic support, we might as well use it...

Lestropie commented 10 years ago

Yep, definitely prefer an explicit option for importing FSL grad files and providing the path for both. Can't believe I didn't think of that at the time. As long as it's in the GradOption group it should be clear what's going on.

Have you thought any more about the other half of #12 i.e. auto-output bvecs/bvals on nifti file creation?

jdtournier commented 10 years ago

A little... I think the best course of action is to avoid trying to do too many things automatically, especially if these are probably not necessary. In the case of issue #12, there are already ways to do this using mrinfo, and I think that's probably the sanest thing to do. I vote we leave things as-is...

Lestropie commented 10 years ago

I agree. The only potential issue I see with not automatically outputting the gradient info is once the registration framework is in place, if the reorientation information from some registration is lost because the user writes the result to nifti. But I guess in that case the relevant command could have its own output grad options.

jdtournier commented 10 years ago

Actually, for rigid registration that shouldn't be a problem (provided we don't change the data layout/strides with respect to the input image), since bvecs are provided with respect to the image axes. It's more a problem for the MRtrix encoding...

Lestropie commented 10 years ago

Unless it's between-volume registration i.e. motion correction. Or as you say, if the layout is modified. But yes, I should probably refrain from technical discussions on a public forum at 1AM :-/

jdtournier commented 10 years ago

Hehe. Sweet dreams.

jdtournier commented 10 years ago

Just had a go at doing this, and created a pull request for it. Hoping for a bit of a code review before merging with master...

jdtournier commented 10 years ago

Closed with commit aba52a4e11d