search

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
294 stars 181 forks source link

DWI metadata fixes for master #3011

Open Lestropie opened 2 months ago

Lestropie commented 2 months ago

Re-implementation of #2917. Closes #2477. Closes #2998. Supersedes #2900. Supersedes #2912.

As mentioned in #2917, these changes are all ultimately bug fixes, so should have been implemented on master from the outset. But because of the mass reformatting changes on dev, I had to transfer most of this changeset manually.

The tool over at https://github.com/Lestropie/DWI_metadata is now containerised, and currently configured to download and test this branch. It shows that with these changes, the only tests not passing are those that take sagittal DWIs from dcm2niix as input, as dcm2niix gets the polarity of its bvecs incorrect in those data.

Lestropie commented 2 months ago

Encountered a trap here that simply checking metadata didn't catch, but shows up when testing all commands. Reading bvecs properly requires knowledge of the header transformation as stored on the filesystem, before MRtrix3 (possibly) does its internal transform realignment. However, if you construct an Image<> class instance, and then attempt to load the corresponding bvecs, then that Image<> class only knows the MRtrix3-interpreted transform, not what was stored on disk.

Prior to a6a4a87814cf1c1be17f0013211a022c02b6f348 (Edit: replaced with 2be257c3f21055ed41a4fc34fc767f25f505e766), this compiled fine specifically because of a non-explicit templated Header copy constructor: one generates an Image<> from a Header, then from the Image<> implicitly produces a Header for DWI gradient table loading. But this would yield a NaN-filled gradient table since the bastardised Header would not have the transform realignment information. I have some recollection of prior discussions about whether generating a Header from an arbitrary ImageType should need to be explicit...

I decided against the option of integrating this internal realignment information into the ImageType class template. In particular this information does not to me make sense to necessitate explicit implementation in Adapter classes. Instead, this realignment information remains a part of the Header class, and commands that must find DWI gradient table information (possibly from the command-line) do so based on a Header class instance. It is nevertheless possible that a developer could erroneously do the Header -> Image<> -> Header process, and then bvecs import will fail. At least now it'll fail with NaNs everywhere rather than producing an erroneous result.

Lestropie commented 4 days ago

I have rectified the failing CI tests, and ensured that the tests over at https://github.com/Lestropie/DWI_metadata still succeed following this fix. The PR is therefore ready for review.