Skip to content

Proof-of-concept for total TRX integration - #3281

Closed
mattcieslak wants to merge 9 commits into
MRtrix3:devfrom
PennLINC:mrtrix-whole-trx
Closed

mattcieslak wants to merge 9 commits into
MRtrix3:devfrom
PennLINC:mrtrix-whole-trx

Conversation

@mattcieslak

Copy link
Copy Markdown

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/.csv files.

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:

# Classic: five files needed to reproduce the connectome
tracks.tck  weights.csv  icvf.tsf  isovf.tsf  assignments.txt

# TRX: one self-contained file
tracks.trx   # contains weights (dps), icvf/isovf (dpv), and group assignments

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:

tcksift2 tracks.trx fods.mif weights        # embeds "weights" dps into tracks.trx
tcksift2 tracks.trx fods.mif weights.csv    # writes external CSV as before
tcksample tracks.trx fa.mif fa              # embeds "fa" dpv into tracks.trx
tcksample tracks.trx fa.mif fa.tsf          # writes external TSF as before
tckmap tracks.trx out.mif -tck_weights_in weights  # reads from embedded dps field
tckmap tracks.tck out.mif -tck_weights_in weights.csv  # unchanged existing behaviour

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 contain if (is_trx_input) branches. Three helper functions do almost all the integration logic:

  • open_tractogram(path, properties) to open TCK or TRX transparently, returning unique_ptr<ReaderInterface<float>>, populating properties["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:

// before
Reader<float> file(argument[0], properties);
TrackLoader loader(file, num_tracks);

// after
auto reader = TRX::open_tractogram(argument[0], properties);
TrackLoader loader(*reader, num_tracks);

This only works because I changed TrackLoader's constructor to accept ReaderInterface<float>& instead of Reader<float>&. Not sure if this is cool.

Gotchas

  • TRX coordinates are RAS+, identical to TCK. VOXEL_TO_RASMM in 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 in tckinfo because it's never applied during reading or writing.
  • Commands that only filter streamlines (namely tckedit, tcksift) use trx::TrxFile::subset_streamlines() to remap all dps/dpv/groups to surviving streamlines automatically.
  • Commands that transform coordinates without changing vertex count (tcktransform TRX→TRX path): modify positions in-place on the loaded TrxFile; all metadata is preserved with no extra infrastructure.
  • For commands that change vertex count (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 rerun tckmap with 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.

@Lestropie

Copy link
Copy Markdown
Member

Similarly to #3265, closing in favour of #3415.

While this PR does take a step in the right direction from #3265 in so far as promoting TRX to have software-package-wide compatibility, there are multiple attributes of the proposed changeset that motivated re-attempting implementation from scratch.

  • This PR introduced exclusively TRX as a new format for support. However it has been a long-term project plan to support a broad spectrum of tractogram data formats (Track files: Implement file format handlers #411). By scoping code base modifications specifically to the support of TRX, it fails to properly encapsulate the concepts that differ between the two formats, which in turn would make it more difficult to add support for other formats in the future. Expanded tractography file format support #3415 took the approach of first establishing the common concepts, then adding support for pre-existing formats. (Indeed it only added TRX support at step 16 of 17 of the initial plan.)

  • I consider support for DPS and DPV sidecar data to be of much higher priority than DPG. The latter is unique to TRX (not present in any other format), but also unlike DPS & DPV has more fundamental incompatibilities with existing MRtrix3 conceptual designs. These I think require more extensive planning and thought about how it should fit into the software ecosystem and philosophy, rather than challenging the generative model to implement support in any way it can. (That's not a unique or personal criticism, just a reality of operating with these models)

  • Extending the CLI of tractogram-related commands to have e.g. ".type_tracks_in().type_directory_in()" is one example of a structural misunderstanding. The ultimate purpose of .type_tracks_in()is not to say "this command can accept files with a.tckextension", even if that's what the code currently does; it is to say "this command accepts as input tractogram data". It is the nature of such tractogram data that now changes: it could have a.trxextension, or it could be a directory. It is therefore the implementation of.type_tracks_in()` that needed to change to reflect the broader filesystem compatibility associated with that type of data.

  • In some respects the code reads as though it promotes TRX to be a core feature of MRtrix3 rather than a filesystem data structure with which it is compatible. As an example, opening a tractogram for reading invokes a function in the TRX namespace, with .tck file support preserved as a fallback erroneously within that namespace. There are additionally pieces of code relating to the processing of streamline and sidecar data unrelated to filesystem interfacing that nevertheless invoke functions within the TRX namespace, or must code branch depending on whether the data belong to a TRX dataset. Project design principles however consider interfacing with the filesystem to load / save data, and internal processing of data (even if those data happen to be memory-mapped), to require strict separation.

  • There is a fundamental design conflict between MRtrix3 and TRX that necessitated recognition up-front in order to get to a suitable implementation (discussed in ENH Support TRX format #2241); the model has tried to find solutions around it rather than addressing it head-on.
    A key benefit of TRX is extensibility: the ability to add and remove sidecar data without necessitating reconstruction of the entire dataset (absent decompression / compression). For software designed for the sole purpose of interfacing with TRX data, this is very convenient: memory-map each file individually, process each streamline by index, access these data per streamline as required. MRtrix3 however has its well-established multi-threading pipeline, where instances of a data structure are loaded from source into RAM, passed down a multi-thread-safe queue where it is read by one of multiple worker threads, and instances of the derivatives of such are potentially passed down another multi-thread-safe queue to be read by a single thread that deals with filesystem output. Preserving this conception therefore requires definition of a data structure that aggregates vertex and sidecar data per streamline into class instances. Then any functor that is intended to operate on tractogram data on a per-streamline basis receives as input all data associated with each streamline in turn, without requiring shared permissions to larger data structures and accessing streamline data by index. This will presumably turn out to be slower than compiled-language implementations tailored specifically to TRX, but is the appropriate design for both separating data input / output from data processing and for supporting a broader set of filesystem formats.

While it won't be supported in this form, this is nevertheless useful content, over and above motivating me to make my own attempt in #3415. Any features that went into the requirements document for this generative exercise that have been overlooked in #3415 can be added; either as comments there, or perhaps better as a standalone Issue listing desired additional features so that implementation can be planned accordingly.
In particular I've not wrapped my head around how best to deal with DPG data. Obviously the nature of those data themselves are fairly trivial, but the exercise of marrying it up appropriately with the pre-existing MRtrix3 code logic and command interfaces is non-trivial. #3415 introduces a new command-line syntax for referencing sidecar data within tractogram datasets, so proposals would likely involve substituting what may currently be specified as standalone input / output files with explicit references to sidecar fields within input / output tractograms.
I also see that you added some features to mrview relating to groups. In #3415 I have some TRX customisations---auto-populating the combo boxes for modifying streamline visualisation with whatever DPS / DPV are present in the input dataset in addition to permitting setting those attributes from external sidecar data, speed-loaders for getting TRX data onto the GPU without internal conversion---but I didn't attempt modifying visualisation by groups. It would be useful to articulate the full set of desired features before diving into implementation.

I'm slowly building up some project-level agent configuration that will eventually be added to the repository. I'm hoping that articulating some of the design philosophies will help mitigate models going down an implementation route that maintainers won't like. Any inputs in this regard from your experience could be useful.

Cheers
Rob

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants