Supporting multiple .tck files

[jhadida]

This PR follows from #1569 and #1567.

It is an enhancement of the existing code to support multiple input .tck files for commands that currently accept .tck files:

• This is done by introducing a new file type with extension .lst (for "list"), containing one filename per line. These can be used either in place of .tck arguments, or in place of weights files (e.g. option -tck_weights_in).
• The changes are completely transparent AFAICT: the behaviour of the software remains unchanged when .lst files are not involved.

Implementation

The changes mainly consist in a re-write of the class Tractography::Reader, which now holds a std::vector of tracks filenames (and optionally of weights filenames too). The ReaderInterface is preserved: the operator() method implemented in Reader simply reads the data off the currently open file(s), and switches to the next file(s) when reaching EOF.

The main challenge in this implementation is to find a consensus between the headers of the multiple .tck files. The proposed implementation mimicks the strategy currently found in tckedit, namely:

• accumulate properties count and total_count;
• aggregate other properties and comments as long as they are the same (or not present) across headers;
• assign the value "variable" in case of conflict.

In addition, the proposed consensus fails if the datatypes varies across files.

Limitations

The intention behind the changes introduced is to avoid unnecessary merge operations, and to provide a seamless capability to handle multiple tracks sources in input. This does not apply to the output; .lst files will not be recognised as output arguments, and any output will still be written to a single file (as before).

This can be troublesome when the output of a command is meant to be used as the input to a subsequent command. For example with the output weights from tcksift2 passed as the -tck_weights_in option of tck2connectome. In that case, at present, the output weights must be manually split into as many .tck files used (i.e. the lines of the .lst file used with tcksift2); and another .lst file should be created listing those new weights files. This is relatively easy to implement in Python, and I would be happy to provide an example if needed.

Alternatively, a split-output behaviour could potentially be implemented within the Writer class, by specifying output names within an .lst file as well (although this might not be relevant for all commands, and implementing a distinction might be cumbersome).

Possible future work

The consensus implemented in tckedit should be replaced with the new implementation in tractography/file_base.

The changes introduced make the implementations of Tractography::Editing:Loader and Tractography::Mapping::Loader somewhat redundant. I think these loader-classes should be merged into a single implementation, in order to assess whether or not they are really needed, given the new capabilities of Tractography::Reader.

[jhadida]

Looks like maximum build-time was exceeded on AppVeyor. On my side, the PR ./build and ./run_tests fine.

[thijsdhollander]

Looks like maximum build-time was exceeded on AppVeyor.

Yep, happens sometimes. It's a bit hit and miss. I just restarted the AppVeyor build. Don't worry about it, it'll get there eventually. Travis CI already tells us that all is well. 😉

[jhadida]

I think there might potentially be a bug with tckedit when run with multiple threads? Something seems to go wrong somewhere either in Editing::Worker or Editing::Receiver...

I will try and put together mcve for this, but in a nutshell; I obtain significantly different connectivity matrices using tck2connectome with option tck_weights_in (SIFT2 weights) when using .lst files on the one hand, or a single .tck file obtained by merging those listed in the .lst file with tckedit.

I have checked thoroughly that this was not due to an error introduced in this PR, or to the way that I split the SIFT2 weights; instead, the only way to obtain the exact same connectivity matrices appears to be by setting -nthreads 0 when running tckedit, which indicates a potential issue with the multi-threaded version. The mismatch between the matrices could be due to rounding issues, but I don't think these would cause such large differences (+/- 5% average variation across edges).

EDIT: It is not due to the way that I split the SIFT2 weights, but to the way that they should be merged... I just realised that merging the smaller .tck files into a larger one with multiple threads does not preserve the order of streamlines (duh)... so the associated SIFT2 weights should also be merged (or rather "un-split" to overwrite the original SIFT2 file) with the same tckedit command, using options tck_weights_in and tck_weights_out.

TL;DR: this is how to merge multiple tracks files with associated SIFT2 weights

tckedit -tck_weights_in SIFT2.lst -tck_weights_out SIFT2_merged.txt input_tracks.lst output.tck

[Lestropie]

This is done by introducing a new file type with extension .lst (for "list")

I'm not sure there needs to be any stringent requirement for such a file extension; for instance, commands like fixelcfestats and mrclusterstats already take as input a text file containing a list of file names, and a .txt file extension would commonly be used for such.

It should be possible to load track data with a series of try/except clauses: Try to load the input as a .tck; if that throws an Exception, try to load the input as a text file containing a list of file names; if that throws an Exception, inform the user that the specified input could not be successfully interpreted as track data input.

The only downside to this I see currently is that it would no longer be possible to perform as stringent sanity checking when an Argument is flagged as .type_tracks_*(), meaning that command-line mistakes may result in an error after some processing has begun rather than at the command-line parsing stage; but to me at least this is a lesser issue.

... the only way to obtain the exact same connectivity matrices appears to be by setting -nthreads 0 when running tckedit

There is already a paragraph in the Description section of multiple commands (including tckedit) that highlights the issue of non-preservation of order when multi-threading. But that description alone is far from an ideal solution. I describe in #502 the necessary modifications to the Tractography::Writer class in order to preserve streamline order in the presence of multi-threading. But as with many things, it simply hasn't made its way up the priority list.

---

There's also been at least one discussion around here somewhere regarding the handling of SIFT2 weights, in particular whether they should be embedded within the track data (technically possible without breaking backward compatibility with the .tck format, but would be kinda clumsy; ideally it'd require a new format), and/or stored as a separate file (my preference at least to this point in time). Having the possibility of multiple track files may or may not influence the relative merits of different approaches, so just making sure that any changes proposed here don't necessarily have to be stringently constrained by such prior decisions.

Indeed in the context of BIDS Derivatives development I raised the prospect of doing something similar to the fixel directory format: Split the header key-value contents into e.g. a JSON file, have track data stored in binary format in some file with a fixed name, and any additional data files (whether providing a value per vertex, like in .tsf, or a value per streamline, which could be streamline weights / Track-Weighted Imaging (TWI) factors / anything else) can be stored within the same directory. In that context, the proposed augmentation here would be the possibility of storing both streamlines vertex data, and other forms of data, within more than one file; the question then would be how to standardise it in such a way that multiple software developers would be willing to support it.

Not necessarily pushing the narrative in one direction or the other here; just exposing some thinking in case it highlights a potential solution that may otherwise be missed.

[jhadida]

Apologies for the mess with force-push / close-reopen; I didn't intend to modify the submodule in training/data but somehow this got into one of the commits, so I had to revert that.

I'm not sure there needs to be any stringent requirement for such a file extension

From my perspective, it's not a matter of need so much as an issue of clarity. Of course, these are just plain text files, and extensions in general are artificial. But let's take the command in my previous message as an example:

tckedit -tck_weights_in SIFT2.lst -tck_weights_out SIFT2_merged.txt \ 
    input_tracks.lst output.tck

would become, replacing .lst with .txt:

tckedit -tck_weights_in SIFT2.txt -tck_weights_out SIFT2_merged.txt \ 
    input_tracks.txt output.tck

It's hard for me to imagine that anyone would find the latter less confusing:

• Is SIFT2.txt a file containing SIFT2 weights or not?
• Is the only way to answer the previous question really to look at the input tracks format? That seems pretty fragile to me.

Having a separate extension makes it clear, even without knowing much, that SIFT2.lst and input_tracks.lst are somehow related. It's not sine qua non in my mind, but I think it's a good idea.

There is already a paragraph in the Description section of multiple commands (including tckedit) that highlights the issue of non-preservation of order when multi-threading.

That was just me being tired; I should have realised this long before I started calling it a bug, sorry. It's a non-trivial gotcha, but the previous command is simple enough to remember, and you only make that mistake once.

There's also been at least one discussion around here somewhere regarding the handling of SIFT2 weights, in particular whether they should be embedded within the track data (...) Split the header key-value contents into e.g. a JSON file, have track data stored in binary format in some file with a fixed name

I think including weights and additional information within the .tck file is a bad idea. The immediate question would be: what else do we include? How would the computation of SIFT2 weights work then? Would we need to "reserve" space within the .tck file before computing the weights? Or would we rewrite the entire .tck file in output with additional information?

I find the simplicity of .tck file really attractive as a contributor: it is to the point, and super easy to parse (I wrote parsers in Matlab, Python and C++ in a few hours). The thought of making it more complicated makes me sad 😿

On the contrary, I 😍 the idea of using folders as data structures. It makes so much more sense to exploit the file-system's ability to structure data hierarchically, rather than say, to work with complicated formats like hdf5 or netcdf which nobody really understands. In one word: transparency. I love this idea, but I wouldn't rush in this direction without first making sure everyone is on the same page.

[Lestropie]

Apologies for the mess with force-push / close-reopen; I didn't intend to modify the submodule in training/data but somehow this got into one of the commits, so I had to revert that.

All good, it's not really that difficult to figure out since everything ends up tracked and cross-linked. Mistakes get made all the time with the testing submodule so no biggie there.

From my perspective, it's not a matter of need so much as an issue of clarity.

No argument against the observation that it's more clear. But we try to not place arbitrary restrictions on the user; the fact that using a text file with any extension would theoretically work, as long as it contains a list of file names (which is easy to check), makes it hard to justify not permitting such; and the fact that there are other existing commands that already accept a text file containing a list of file names where no such extension restriction exists means that the issue is more likely to be encountered by / confusing for users.

One advantage gained from such a restriction is the ability to do a preliminary check on the user input during command-line parsing. However this would likely be altered upon addressing #411 anyway: I'd expect that something similar to image format handling would occur, where each format is queried in turn to see if it can read the input data; a handler that interprets a list of file names would simply be one particular instance of such, and would require a preliminary scan of the contents of the file, just as is required when a directory is provided as an input image to determine whether or not it contains DICOM data.

Is SIFT2.txt a file containing SIFT2 weights or not? ... Is the only way to answer the previous question really to look at the input tracks format?

Technically, as long as the file either contains a vector of values, or a list of file names (each of which contains a vector of values), whether or not the input track data are stored in a single file or across multiple files (and whether or not that is consistent with how the weights data are stored) is not actually crucial for reading in the weights. There are already warnings issued if the number of streamline weights read is more or less than the number of streamlines, which would be enough to catch egregious misuses. Beyond that, the only issue is whether or not the order of the data are consistent between tracks and weights, and that is up to the user to manage. But I'm not sure that the weights data should have to be split across multiple files if that's how the track data are stored, nor vice-versa; both can be reasonably read in isolation, with having the same cardinality in both sources being the only stringent constraint.

I suppose the ambiguity that this would raise would be running the tcksift2 command on a list of track files, and specifying the output text file as e.g. a .txt: Should the weights be written directly to such a file, or should the output be a text file containing a list of file names? What if the latter leads to an attempt to overwrite an existing file based on auto-generated filenames for the output weights to match the input track file names? This could be controlled via a config file option, but even choosing what should be default is ambiguous. Indeed this is one aspect where instead relying on tckedit to do a concatenation and piping the data to tcksift2 would be less ambiguous; but I suppose in that case you'd then need to have knowledge of the order in which those files were fed to tckedit in order to interpret the weights correctly... :-/

That was just me being tired; I should have realised this long before I started calling it a bug, sorry. It's a non-trivial gotcha, but the previous command is simple enough to remember, and you only make that mistake once.

Well, I'm not wholly convinced: I don't know how common it might be that users run their commands as such and simply don't interrogate the output very closely. The description section is not very "loud". I'd feel much more comfortable with the underlying mechanism resolved... @jdtournier worth bumping for RC4?

I think including weights and additional information within the .tck file is a bad idea.

I agree. I don't recall exactly where the discussion took place, but essentially the observation was that each streamline is delimited by a triplet of NaNs, so one of those three NaNs could be "co-opted" to carry the streamline weight, and the delimitation would still be detectable. But:

• It's clumsy;

• Would have the potential to break external parsing software written based on the current specification;

• Would mean for tcksift2 output either editing a .tck file in-place (not ideal) or making a copy of the .tck file containing the weights (occupying more HDD space than saving the weights in isolation; and we know how you feel about redundant HDD data 🤣 (jks));

• A command-line flag for using or not using the weights in any particular command invocation would still need to be provided anywhere that -tck_weights_in exists currently, or there'd need to be some mechanism to "strip" the weights from track data in order to be able to interpret said track data in the absence of per-streamline weighting.

I love this idea, but I wouldn't rush in this direction without first making sure everyone is on the same page.

If that does get pursued, I would expect that it would be in the BIDS Derivatives context rather than the MRtrix3 context.

[Lestropie]

Similarly to #3281, I think this one is going to be dropped in favour of #3415.

#3415 includes a tractogram "file format" that involves sending binary data through Unix pipes. This enables the tckedit concatenation followed by piping to another command that was suggested earlier here.

With other goings-on in #3415 it then additionally has the capability to integrate data-per-streamline and data-per-vertex into the content of that piped data. This alleviates the issue recognised with the approach implemented here in that one may want to concatenate both streamline vertex and sidecar data, and the order of concatenation must be consistent and there should be some convenient interface by which to do so. If those sidecar data are embedded within the tractogram file format for each dataset individually then they should be preserved through the concatenation operation, and each dataset must only be specified once which mitigates against ordering errors.

The TRX format that motivated the effort in #3415 also makes use of the concept raised here of embedding tractogram sidecar within a directory structure (or indeed compressed / uncompressed tarballs), which can alleviate sidecar data correspondence issues similar to those discussed here in the absence of dataset concatenation.

If it were to be the case that despite changes in #3415 there is still nevertheless utility in having the ability to specify "an input tractogram" that is actually a text file containing a list of tractogram files to be treated as a singular dataset, then rather than attempting to integrate the change here, that should instead be implemented from scratch as an extension of #3415. It would actually be far more elegant there.

• Define new tractogram format handler for text files containing a list of tractograms
  • Format attributes: read-only, speculatively compatible with embedded sidecar data depending on the formats of the referenced files, not memory-mappable
• Implement check function for testing whether a user-provided file can be interpreted as containing filesystem paths to tractogram files, in which case the format handler claims responsibility for parsing
• Perform an initial speculative load of all files, invoking the appropriate tractogram format handler for each, to determine the header information and sidecar data structure representing the aggregated dataset
• The format functor that yields one streamline at a time simply invokes the same functor for member variable containing a Tractogram class initialised by a path stored in the text file
  • Delete and re-construct that member variable as one tractogram file is exhausted and the next must be read
  • Remove from the yielded class any sidecar content that, while present in that particular input tractogram dataset, is not consistently present across all datasets referenced in the file.