Skip to content

Add Norm and Mode of Anisotropy (NA and MO)#3269

Merged
Lestropie merged 6 commits intoMRtrix3:devfrom
kikiluvbrains:add_mo
Apr 9, 2026
Merged

Add Norm and Mode of Anisotropy (NA and MO)#3269
Lestropie merged 6 commits intoMRtrix3:devfrom
kikiluvbrains:add_mo

Conversation

@kikiluvbrains
Copy link
Copy Markdown

@kikiluvbrains kikiluvbrains commented Feb 19, 2026

Adds NA and MO as an output option in tensor2metric. Verified using the provided test data and an external DTI dataset.

paper link: Ennis, D. B., & Kindlmann, G. (2006). Orthogonal tensor invariants and the analysis of diffusion tensor magnetic resonance images. Magnetic resonance in medicine, 55(1), 136–146. https://doi.org/10.1002/mrm.20741

other supporting literature:
Chad, J. A., Pasternak, O., & Chen, J. J. (2021). Orthogonal moment diffusion tensor decomposition reveals age-related degeneration patterns in complex fiber architecture. Neurobiology of aging, 101, 150–159. https://doi.org/10.1016/j.neurobiolaging.2020.12.020

example of mo on example dti data:
image

@kikiluvbrains kikiluvbrains changed the title Add mo Add Norm and Mode of Anisotropy (NA and MO) Feb 24, 2026
Comment thread cmd/tensor2metric.cpp Outdated
Comment thread cmd/tensor2metric.cpp Outdated
Comment thread cmd/tensor2metric.cpp Outdated
Comment thread cmd/tensor2metric.cpp Outdated
Comment thread src/dwi/tensor.h Outdated
Comment thread src/dwi/tensor.h Outdated
@Lestropie
Copy link
Copy Markdown
Member

Lestropie commented Mar 3, 2026

Thanks for the contribution @kikiluvbrains! Unless there's contention about the metrics themselves this should hopefully not be too difficult to accept.

  • PR currently shows unrelated commits. Hopefully this is just due to the merge conflicts with dev, and doing an explicit merge will resolve.
    (Let us know if you need help in this regard)

  • The new contrasts should be computed from the test DWI data and committed to the test data repository, and tests should be added to check for any regression.
    (This is kind of clunky to deal with as it's a separate repository, so I'll do it myself)

  • Address code comments.
    (If you want to accept multiple suggested changes as-is, please aggregate them into a batch; otherwise just edit and commit as you see fit)

@kikiluvbrains
Copy link
Copy Markdown
Author

kikiluvbrains commented Mar 29, 2026

Hey, merge conflicts should be resolved by now. And I went through and added in all fixes from the comments. Now, I think we need to add the new contrasts for the test DWI data.

And by test data do you mean this I have the output on NA and MO from the test dt.mif file, I can upload those in if that would help

@Lestropie
Copy link
Copy Markdown
Member

Ah, there's a bigger discrepancy here that I didn't realise in my first look.

While the PR is currently targeting the dev branch---which is the right choice given that the contribution is a feature addition and not a bug fix---it seems that the version of the code to which the code modifications were first applied was either the master branch or something close to it. This is a more substantial distinction at this point in time than it would have been previously, because of major changes that have happened on the dev branch. The command code has not only changed location (from cmd/ to cpp/cmd/), but has also undergone a mass code reformatting. This tends to result in attempts to move code between those two branches (or their children) being quite difficult, because git labels absolutely everything as a conflict. As per the contribution documentation, resistance is minimised if the correct starting branch is chosen from the outset before writing any code.

Given the disproportionate nature of the consequences due to that discrepancy, I think the best course is if I deal with the conflicts myself on this one. While it's useful to learn about resolution of merge conflicts in git, this is perhaps not the right use case to be learning from. There's probably also commits on master not yet back-propagated to dev that need to be dealt with separately so that the unrelated commits disappear from this PR.

Conflicts:
	cmd/shconv.cpp
	cmd/tensor2metric.cpp
	core/stride.cpp
	cpp/core/dwi/tractography/editing/loader.h
	docs/reference/commands/shconv.rst
	src/dwi/tensor.h
@Lestropie
Copy link
Copy Markdown
Member

Lestropie commented Apr 2, 2026

OK, I've done a bit of work here:

  1. Merged your changes onto the dev branch.
    Took me a couple of tries to get right...

  2. Did a little bit of refactoring.
    There were a couple of suggestions I made that were only applied to the code relating to one of the two metrics rather than both.
    I additionally made use of Eigen functionalities in the computation of those metrics. Admittedly you would have been looking at the existing code in tensor.h for precedence, and that code doesn't really use Eigen, but that's because it's very old code that predates our change to using Eigen. So it's most certainly not a mistake on your part. But I nevertheless took the opportunity to demonstrate how Eigen's capabilities can be used to reduce the overall code size and arguably better communicate the intent of the calculation.

    Edit: This refactoring is shown in commit d081113.

  3. Added test data and tests.
    As mentioned previously, this is clunky, so there wasn't any point in me trying to provide instruction from scratch, but hopefully it can demonstrate how it works such that if you make other contributions you might be able to complete part of the process yourself.
    The addition of the data itself is over on another repository:
    https://github.com/MRtrix3/test_data/tree/add_mo
    This code repository then needs to be instructed to pull that new data. The way in which this is encoded is different between the current master and dev branches; in 9021d51 you can see the update to testing/CMakeLists.txt. The addition of a test to generate those contrasts and compare against the pre-computed version is also slightly different between master and dev. dev is nicer IMO because each individual test is its own file, which enables more complex tests and the addition of comments.
    Once this PR is merged into dev, I then have to go back to the test_data repository, update the dev branch there to point to the commit with the new data, and delete the new add_mo branch there.
    So a lot of juggling, and a lot of ways for it to go wrong. I stuff it up all the time myself. And it requires write access to that test data repo. Hence why it's easiest for me to just do it. But certainly the writing of new tests is something that external contributors can have a go at themselves.

    (It's not this difficult a process to add test data to most software packages. Ours is harder because the test data are quite large and we don't want to bundle it all with every clone of the software. Hence there's actually two external repos that host the test data. But this then requires manually keeping them in sync with one another, which requires effort and diligence.)

@Lestropie Lestropie merged commit b69b55c into MRtrix3:dev Apr 9, 2026
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.

3 participants