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.

Sign up for GitHub

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

Jump to bottom

DWI metadata fixes for master #3011

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

DWI metadata fixes for master #3011

wants to merge 10 commits into from

Conversation

Lestropie
Copy link
Member

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
-json_export: Fix slice encoding direction sign
ca55d14
@Lestropie
-export_grad_fsl fix for highly non-RAS acquisitions
a5a76ad
@Lestropie
DWI metadata fixes
339ea0c 
This commit is a manual replication of the contents of #2917, but targeting 3.0.x.
@Lestropie
JSON export: More exclusion of gradient table
7dfb6ec
@Lestropie
JSON import: Preserve precision of numerical types
b160d21 
Do not convert contents of text JSON file to integers / floats then convert back to strings for storage in a KeyValues instance; instead just use the string representation. This fixes an issue whereby if a JSON is generated from an image, but that image also retains the same metadata fields in its own header, and then the JSON contents are re-imported, the two values for floating-point metadata elements could differ due to floating-point imprecision, and this would lead to the field being reset to the value "variable".
@Lestropie
Merge remote-tracking branch 'origin/master' into dwi_metadata_master
01a09b8
@Lestropie Lestropie added the bug label Sep 20, 2024
@Lestropie Lestropie added this to the 3.0.5 updates milestone Sep 20, 2024
@Lestropie Lestropie requested a review from a team September 20, 2024 23:39
@Lestropie Lestropie self-assigned this Sep 20, 2024
This was referenced Sep 20, 2024
Closed
Closed
@Lestropie Lestropie marked this pull request as draft September 22, 2024 01:03
Lestropie added a commit that referenced this pull request Sep 22, 2024
@Lestropie
Fixes to bvecs import
a6a4a87 
In order for a bvecs file to be properly read in, the information about how the image was transformed upon image load needs to be available. However if an attempt is made to perform such a read based on a Header that it has itself been constructed from an arbitrary ImageType, this information is lost. Therefore, reading from a bvecs file needs to be done using the Header class in which the corresponding NIfTI was originally opened.
In the process, some functionalities of the tracking of that transformation on image load have been made more explicit.
Also fixes some syntax errors relating to the manual back-porting of #2917 in #3011.
@Lestropie
Copy link
Member Author

Lestropie commented Sep 22, 2024 •
edited
Loading

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 a6a4a87 (Edit: replaced with 2be257c), 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 added a commit that referenced this pull request Sep 22, 2024
@Lestropie
Fixes to bvecs import
178f1cc 
In order for a bvecs file to be properly read in, the information about how the image was transformed upon image load needs to be available. However if an attempt is made to perform such a read based on a Header that it has itself been constructed from an arbitrary ImageType, this information is lost. Therefore, reading from a bvecs file needs to be done using the Header class in which the corresponding NIfTI was originally opened.
In the process, some functionalities of the tracking of that transformation on image load have been made more explicit.
Also fixes some syntax errors relating to the manual back-porting of #2917 in #3011.
@Lestropie
Fixes to bvecs import
2be257c 
In order for a bvecs file to be properly read in, the information about how the image was transformed upon image load needs to be available. However if an attempt is made to perform such a read based on a Header that it has itself been constructed from an arbitrary ImageType, this information is lost. Therefore, reading from a bvecs file needs to be done using the Header class in which the corresponding NIfTI was originally opened.
In the process, some functionalities of the tracking of that transformation on image load have been made more explicit.
Also fixes some syntax errors relating to the manual back-porting of #2917 in #3011.
@Lestropie Lestropie mentioned this pull request Sep 26, 2024
Open
@Lestropie Lestropie mentioned this pull request Oct 10, 2024
Closed
@Lestropie
Handle inconsistent phase encoding on non-volume concatenation
fc0e8f1 
Concatenation of images along an axis other than 3, where phase encoding information differed, results in "PhaseEncodingDirection": "variable". Previously this was dissapearing silently; but with #3011, this instead threw an unhandled Exception.
@Lestropie Lestropie marked this pull request as ready for review November 17, 2024 13:07
@Lestropie
Copy link
Member Author

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.

Lestropie added a commit to Lestropie/DWI_metadata that referenced this pull request Nov 17, 2024
@Lestropie
Multiple fixes
41dff10 
- Pull in latest code changes at MRtrix3/mrtrix3#3011 to ensure no regression has occurred in fixing other issues.
- Fix invocation of peakscheck for dwi2tensor outputs.
- Fix container entrypoint.
- Fix README to include invocation of container and reflect fix to container entrypoint.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Lestropie Lestropie

Labels
bug
Projects
None yet
Milestone
3.0.5 updates
Development

Successfully merging this pull request may close these issues.

mrconvert: fslgrad and RealignTransform: 0 DWI metadata fixes on master
1 participant
@Lestropie