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

5TTgen MSMT #3025

Open arkiev opened 1 month ago

arkiev commented 1 month ago

This script is intended for generating a 5-Tissue Type (5TT) image from Orientation Distribution Function (ODF) images in the MRtrix3 neuroimaging framework.

By default, white-matter, grey-matter, cerebrospinal fluid ODFs along with a binary brain mask is used to generate a 5TT image with empty volumes for subcortical grey-matter and pathological tissue. If SGM and/or PATH segmentations are available, there is an option to include them in the 5TT image.

Lestropie commented 1 month ago
Lestropie commented 1 month ago

Rather than accepting suggestions individually, can you please add them to a batch commit; otherwise the commit history becomes non-informative.

arkiev commented 1 month ago
Lestropie commented 1 month ago

I'll have to brush up on my git rebase skills to combine those suggestion acceptances.

For checking images being on the same voxel grid see image.match().

Did you duplicate checkboxes because you weren't able to activate the ones I posted? I could imagine GitHub might not give you permission to do that.

arkiev commented 1 month ago

thanks for sharing image.match() which has been incorporated

correct - wasn't able to tick off your checklist

Lestropie commented 1 month ago

Rebased to clean up commit history. For GitHub code reviews with a lot of minor changes in different locations of the code you can add the agreed-upon changes to a batch, and GitHub will then create a single commit containing all such changes with appropriate attribution: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request

Lestropie commented 1 month ago

@arkiev I had a bit of a go at the code here. Sorry if I ended up hacking away much of it. But hopefully there's a few teachable things in there:

arkiev commented 1 month ago

Thanks for the feedback @Lestropie. It is looking good!!

I address the errors from pylint - there was another error that I wasn't familiar with (I think it was a build error, but since my last commit a new set of checks needs to be done to reproduce the error). How do I run those checks (or can I leave this with you)?

Lestropie commented 1 month ago

Looks like in 2bc20890729d43d3216ffa4e2932e2772700c8ee you did a find & replace from " to ' and inadvertently edited the copyright header in the process, which now fails tests :rofl:

The clang-tidy failure is irrelevant to this PR. Might be spontaneous and just need a re-run at a later date. dev isn't locked to CI passing so I can merge it anyway if I want to. But I want to add test data and tests first.

arkiev commented 1 month ago

ah yes I did do that 😅

Okay, sounds good - let me know what is needed from my end for testing.

At some point we should think about comparing the SGM segmentation generated using the vis image (using run_first_all <vis> followed by 5ttedit) against the ideal way to create the population template SGM (create subject-specific SGM segmentation, apply the warp to get the segmentation into template space, compute the average across subjects). If they are comparable, perhaps include an option to do the SGM segmentation generated using the vis image.

Lestropie commented 1 month ago

dddce99904b75037d76ea1851067cce912d75ed0 shows how you can add tests. These aren't evaluated in CI, but you can run them if you configure CMake accordingly. Also see how I had to commit data to a different repo and then update the reference; this is now a bit different on dev---see updated instructions here---so I did this one myself.

If you want to add the ability to invoke run_first_all on the pseudo-T1w image and integrate its output into the 5TT image, that can probably be proposed as a separate augmentation. It's not a trivial addition, it would be preferred to make use of #2558, and it would need its own additional test (which you can now see how to do). So this PR could be merged in the absence of such. I'm a little hesitant about adding to MRtrix3 itself even more scripts that invoke non-MRtrix3 functionalities; I'd kinda prefer that any such things go into a separate repository. Here that functionality doesn't strictly need to be in the same script, even it would just be more convenient for it to be there. You can take a 5TT image generated by this command and go 5tt2vis -> run_first_all -> 5ttedit as a little standalone modifier script.

RE segmenting SGM in each subject and transforming those results to template space and aggregating, I would conceptualise that as a separate discussion to what does or does not get included as an MRtrix3 software feature. Comparability of the two outcomes isn't the only thing determining whether that feature would be useful. Recall 5tt msmt can be run on single-subject data.