Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

5TTgen MSMT #3025

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

5TTgen MSMT #3025

wants to merge 10 commits into from

Conversation

arkiev
Copy link

@arkiev arkiev commented Oct 10, 2024

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
Copy link
Member

Lestropie commented Oct 11, 2024

  • I would consider renaming to "5ttgen msmt", given it's specifically information from a multi-tissue decomposition that is used to generate a 5TT image; a single ODF couldn't be used.

  • Input mask could technically be made a command-line option rather than a compulsory positional argument. One way to do it would be similar to how dwibiasnormmask operates. Generate a mask that contains only those voxels where the sum of ODF l=0 terms exceeds 0.5/sqrt(4pi); then select the largest component and fill any holes. A user-specified mask acts as a substitute for such.

  • Standard indentation for MRtrix3 is two spaces, not four.

  • Need to run docs/generate_user_docs.sh and contribute changes.

  • Currently mrgrid regrid is always run on the input mask image. Better would be to detect whether or not the input mask image already uses the same voxel grid as the input ODFs. If it does, then a straight multiplication can be done. If not, then a warning should be issued; then, marginally preferable to a nearest-neighbour regridding is to instead do a resampling of the input binary image and then threshold the result at 0.5.

  • Should be an explicit check regarding whether the input ODF images are stored on the same voxel grid, prior to attempting to concatenate them.

  • For SGM and "pathology" tissue types, technically the output of 5ttgen when run with this algorithm could be piped directly to 5ttedit. So I'm wondering if it would actually be better to remove that functionality from here.

@arkiev arkiev marked this pull request as ready for review October 11, 2024 02:59
@Lestropie
Copy link
Member

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

@arkiev
Copy link
Author

arkiev commented Oct 11, 2024

  • name has been changed to msmt.py
  • mask changed to optional input. By default, brain mask created using the dwibiasnormmask approach: mrcalc totalvol.mif 0.1410473959 -gt - | maskfilter - clean - | maskfilter - fill - | mrcalc - fTT_dirty.mif -mult result.mif
  • indentation updated to two spaces
  • Still need to run docs/generate_user_docs.sh and contribute changes
  • mrgrid regrid is only conducted on input mask if input mask has different spacing to the 5TT image, and nearest neighbour interpolation has been replaced by resampling and thresholding: mrgrid {app.ARGS.mask} -template wm_vol.mif regrid - | mrcalc - 0.5 -gt - | mrcalc - fTT_dirty.mif -mult result.mif
  • still to do explicit check regarding whether the input ODF images are stored on the same voxel grid, prior to attempting to concatenate them. Can a simple comparison of mrinfo -size be used here, or is there more to it?
  • SGM and PATH have been removed

@Lestropie
Copy link
Member

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 arkiev changed the title 5TTgen ODF 5TTgen MSMT Oct 15, 2024
@arkiev
Copy link
Author

arkiev commented Oct 15, 2024

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

correct - wasn't able to tick off your checklist

@Lestropie
Copy link
Member

Lestropie commented Oct 20, 2024

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

  • Check dimensionalities of ODFs

    • Issue warning if WM ODF is lmax=0, but can technically still proceed
    • Issue warning if GM ODF is lmax>0, but can technically still proceed
    • Issue warning if CSF ODF is lmax>0, but can technically still proceed
  • Combine some operations to reduce scratch storage

  • Add description to explain how brain masking will operate in the absence of an input brain mask

  • Derive some suitable test data & add tests

@Lestropie
Copy link
Member

@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:

  • Avoid code duplication wherever possible. If there's three lines of code that are almost identical except for some small change to a string, do it in a loop or list comprehension or the like.

  • I nowadays try to avoid having filesystem paths as strings that get duplicated across multiple command invocations. It's too prone to error. So I try to store them in named variables.

  • While it's possible to run a series of commands and store all of the intermediate images in the scratch directory, I'm conscious of MRtrix scratch directories blowing out in size, and unnecessarily using hard drives. So I try to make use of the fact that run.command() can deal with image piping, and the underlying MRtrix commands might be able to instead use a RAM filesystem for the intermediate data. The actual command or set of commands to be executed can be constructed according to more complex underlying logic.

@arkiev
Copy link
Author

arkiev commented Oct 21, 2024

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
Copy link
Member

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

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
Copy link
Author

arkiev commented Oct 21, 2024

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
Copy link
Member

dddce99 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants