Proof-of-concept for total TRX integration#3281
Open
mattcieslak wants to merge 9 commits intoMRtrix3:devfrom
Open
Proof-of-concept for total TRX integration#3281mattcieslak wants to merge 9 commits intoMRtrix3:devfrom
mattcieslak wants to merge 9 commits intoMRtrix3:devfrom
Conversation
d5fc100 to
bbe79ef
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I tried adding TRX support in #3265 and realized that there are a lot of TRX-related functions that should probably be reusable outside of tckconvert.cpp. So I tried out adding TRX support across all of mrtrix and was able to get it working and passing old and new tests. In this PR, every command that accepted TCK now accepts TRX, with (mostly) no new required CLI options. Many commands gain the ability to embed computed sidecar data (weights, per-vertex scalars, atlas labels) directly into the TRX file rather than scattering it across separate
.txt/.tsf/.csvfiles.I did some benchmarking of TRX-backed vs classic MRtrix3 workflows here.
Big picture
A single TRX file accumulates all the data classically split across multiple sidecar files:
The strategy I took is that a sidecar output argument (weights path, TSF path, scalar dump path) doubles as a TRX field name when given without a file extension. For example:
All existing behavior is preserved when an extension is present or the input is TCK.
Specifics
All of the trx-specific handling is implemented in
trx_utils.h, not in command code. Commands do not containif (is_trx_input)branches. Three helper functions do almost all the integration logic:open_tractogram(path, properties)to open TCK or TRX transparently, returningunique_ptr<ReaderInterface<float>>, populatingproperties["count"].resolve_dps_weights(path, field_name_or_path)— returns per-streamline weights from an embedded dps field or external text file.resolve_dpv_scalars(path, field_name_or_path)— same for per-vertex scalars.Most CLI programs only need two line substitutions:
This only works because I changed
TrackLoader's constructor to acceptReaderInterface<float>&instead ofReader<float>&. Not sure if this is cool.Gotchas
VOXEL_TO_RASMMin the trx header is informational only. It's really only useful to convert to trk, which I hope goes away someday. I kind of want to not even print it intckinfobecause it's never applied during reading or writing.tckedit,tcksift) usetrx::TrxFile::subset_streamlines()to remap all dps/dpv/groups to surviving streamlines automatically.tcktransformTRX→TRX path): modify positions in-place on the loadedTrxFile; all metadata is preserved with no extra infrastructure.tckresample), dps and groups are copied to the new file. dpv is discarded with a warning (vertex-count change invalidates per-vertex data). If users want to get dpv back they could reruntckmapwith the resampled trx.Critical Note
I want to be clear this is just a proof of concept showing what TRX integration could look like, not a proposal to merge exactly this code. I'm pretty satisfied with the MRtrix3-TRX workflow in this PR but obviously am a newbie to MRtrix3 development and c++, and want to be a good citizen. I'm happy to implement any suggestions here or in separate PRs!
Also, it's worth mentioning that it will be extremely nice to be able to mix and match software tools. We will soon be able to
antsApplyTransformsToTRX, read/write streamlines with DSI Studio and are already able to use trx throughout trekker, ITK and in Python/DIPY.