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.

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

[ENH] Use TSV for DWI gradients, and allow both voxel- and world- coordinates conventions #352

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

Conversation

mattcieslak
Copy link
Contributor

Adds a dwi tsv option with gradient directions in physical coordinates instead of image axes. Addresses #349.

@mattcieslak
Copy link
Contributor Author

I added a table describing BigDelta and SmallDelta. These address @arokem's suggestion #349 (comment). Although I'm not sure if the thread is suggesting a json sidecar for the gradient tsv or adding some additional fields to the dwi's json sidecar. This adds them to the existing sidecar

Copy link
Collaborator

@oesteban oesteban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a good step towards addressing #349. There are two main missing items:

  1. Implementation of tensor b-encoding; proposed by @jhlegarreta [ENH] Optional alternative to bval/bvec #349 (comment)
  2. The general discussion about backward compatibility introduced by @chrisgorgo - [ENH] Optional alternative to bval/bvec #349 (comment)

While 1) probably boils down to writing one example, I think 2) deserves careful discussion.

Let me bring here Chris' suggestions:

  1. Require the metadata in either new or old format.

    • pros: old datasets are still compatible
    • cons: old BIDS apps need to be updated to support the new format, new BIDS apps need to support two formats.
  2. Make new format optional (old format remains compulsory):

    • pros: old datasets are still compatible
    • cons: BIDS apps will have to keep supporting the old format
  3. Make old format optional (new format becomes compulsory):

    • pros: new BIDS apps can take advantage of the new format without having to support the old one
    • cons: old BIDS apps need to be updated, old datasets need to be updated
  4. Make old format unsupported (new format becomes compulsory):

    • pros: new BIDS apps can take advantage of the new format without having to support the old one
    • cons: old BIDS apps need to be updated, old datasets need to be updated

Option 3 could lead to 4 after some depreciation period. Tooling could be built to assist in updating datasets.

Chris framed this in the general BIDS scope, mentioning that the question of how to introduce the new format can be separate from the question of what is the ideal new format. Admins might want to split the discussions for clarity.; however, I think this is a special case.

I am pretty confident we should go with Option 3 followed by 4 (Chris' final comment). In this case, the problem is not just a new format replacing the standard format currently in BIDS. The problem is that the old format (bvec/bval) violates some principles of BIDS and has been shown to be really unreliable (the DWI community is always wary of whether their bvecs are x-flipped, for instance). So I was really pleased by @mattcieslak proposal when I first read it. The tsv format is not just more consistent with BIDS, it is also more reliable to define unequivocally the b-matrix.

@oesteban
Copy link
Collaborator

There was also some discussion about existing software (and particularly BIDS-Apps) that could potentially require changes if this PR were accepted.

Some people who might want to have a say here: @bids-standard/derivatives-mri-dwi, @ecaruyer, @jelleveraart.

@mattcieslak
Copy link
Contributor Author

Since the discussion has moved here, I asked friends who are using gradient tensor imaging how they store b-tensors and they pointed me to this repository: https://github.com/markus-nilsson/fwf_header_tools. I haven't gone through it in detail yet.

@jhlegarreta
Copy link
Contributor

Thanks for summarizing what has been discussed in the pas few days @oesteban .

Implementation of tensor b-encoding; proposed by @jhlegarreta #349 (comment)

Following the link posted by @mattcieslak, I found @filip-szczepankiewicz 's repository's resources section, which seems indeed extremely useful to have freely available data examples, examples of the sequences and tools to deal with them. Have not investigated the implementation (I am not an expert on FWF/not a user yet), what the gradient data looks like, or how these fit into the TSV+JSON philosophy.

The general discussion about backward compatibility introduced by @chrisgorgo - #349 (comment)

I also see 3 followed by 4 as the way to go.

@chrisgorgo
Copy link
Contributor

+1 for option 3 followed by 4.

Practically speaking it would be a form of deprecation, with the validator issuing deprecation warnings (that ideally will say when the old format will become unsupported), right?

If this works for this case it might be a general way to approach backward-incompatible changes on a case by case basis instead of waiting for 2.0.

@jchoude
Copy link
Contributor

jchoude commented Nov 11, 2019

Hi all,

I'm a bit late on this. I wanted to comment on the initial issue , but it is now locked, so I'll comment here, from a few points that were touched:

  • w.r.t keeping track of the transformations that might have been applied to the gradients, as @arokem mentionned, that would be more suited to the Derivatives spec. However, even there, it isn't quite clear if all steps should be encoded in the .json file. This is more provenance, and less about a user being able to correctly interpret and use the current files.
  • Everyone seems to agree that gradients should be defined in the scanner RAS+ space. I also agree. I think it should be said even more explicitly in the current proposed PR, to ensure that no ambiguity is left.
  • About RAS+ in scanner space: a lot of tools implicitly work with FSL-style encodings. To be able to still work, they would need to transform the RAS+ .tsv values back to FSL-style orientation. This means they would need to know the transform. However, I'm not clear if what is proposed here is to encode the "requested" directions (i.e. specified in the MRI interface), or the effective directions. In some cases and machines, requested directions are rotated to be aligned with the main axes of the subject's head. In this case, which directions would be saved in the .tsv file?
  • About b-tensor style encoding, I think there is still lots of discussion to be had for this point. Considering it is still quite "early" in this field, we might want to differ decision on b-tensor encoding for a future discussion. For example, I'm not sure what would be preferable: saving the analytical trajectories that were acquired, or the effective "gradient steps" that were used by the machine to traverse said trajectory? Maybe we would need to save both, or the analytical version + some information that could be used to generate the steps if needed, such as the number of steps, etc.

Finally, in the initial discussion, there was one big point that was about deprecation, coexisting .tsv and .bval/.bvec. I think we should try to see what timeframe will exist for the cohabitation. If the "next" version that deprecates .bval/.bvec will not happen for quite a while, we run the risk of developpers having to support both types for quite a while, and that might not be optimal.

Thanks for this, I think it will be of grat help!

@oesteban
Copy link
Collaborator

I have rebased this on top of #624, following conversations there. For a clean diff see oesteban/bids-specification@enh/dwi-combinations...mattcieslak:dwi_tsv

@oesteban
Copy link
Collaborator

oesteban commented Apr 5, 2021

@bids-standard/raw-mri-dwi @bids-standard/raw-mri I have recently received a notification for this (I believe) alternative BEP in development - https://arxiv.org/pdf/2103.14485.pdf which I think addresses at the very least some of the problems we are targeting here.

WDYT about the preprint overall, @mattcieslak? should we just close this and ask the authors of the preprint to submit their proposal as a PR?

@francopestilli
Copy link
Collaborator

@arokem, @francopestilli, @Lestropie, @mattcieslak, @satra, @edickie @dlevitas at a first glance the proposal seems good. It also appears to propose additions to the current spec and not necessarily steps towards what we were trying to do (from my point of view). I will read it in more detail, but what do the others think?

@arokem
Copy link
Collaborator

arokem commented Apr 6, 2021

Looks like it very thoroughly addresses the comment on the issue that precipitated this PR. It would be nice to start thinking about these things.

should we just close this and ask the authors of the preprint to submit their proposal as a PR?

I agree with @francopestilli's observation that this might be somewhat orthogonal to what is done here. Many of the things that are proposed are also not crucial for many methods. Maybe this PR could be completed in parallel (or first?)? I think there might be ways in which incorporating their ideas could build upon what is proposed here.

@mattcieslak
Copy link
Contributor Author

The preprint looks very ambitious and covers diffusion encoding schemes that are still new and fairly rare. The main goal of this PR is to have a definite mapping between gradient directions and LR/AP/IS that is independent of the image axis. This should be easy in most cases to get out of the dicoms and fits nicely into a simple tsv, without adding lots of other features

@oesteban
Copy link
Collaborator

oesteban commented Apr 6, 2021 via email

@francopestilli
Copy link
Collaborator

@oesteban @arokem @mattcieslak it looks like we agree. Interesting but orthogonal. A BIDS for Pros.

@JAgho
Copy link

JAgho commented Apr 6, 2021

Hi all,

I'd planned to share this preprint explicitly rather than indirectly. Overcoming difficulties such as the subject of this PR is intended as a central aim of this BEP. Recording parametric variation throughout a diffusion experiment is a necessary evil, and while stopgap solutions resolve temporary issues, ever more intricate sequences promise ever more detailed parametric variation. We've tried to show in the manuscript how the many types of diffusion experiment cannot all be easily expressed in the same format of tabular file, and hence justify the modifiable structure proposed.

We've structured things to suit option 2) here, extending and not superceding the bval / bvec system, which is used in a lot of pipelines we'd like to keep working. In general, the discussion here echoes a lot of conversations we've had internally, concerning paired tsv / json structures. Ultimately, this BEP would address this concern, and from the user's perspective, will mirror the suggestion proposed (of a more detailed tsv with more detailed parametric variation).

This proposal isn't finished until it's seriously reviewed, so any analysis you offer is really appreciated. Please feel free to write to me with any suggestions.

@oesteban
Copy link
Collaborator

oesteban commented Apr 7, 2021

Thanks @JAgho for confirming that your BEP would extend/continue from this one. That clarifies the fate of this particular PR even further.

Regarding the preprint, I think the BIDS steering committee and/or maintainers will reach out to you - particularly large BEPs typically require some discussion. Therefore, ensuring a flow of bi-directional feedback between BEP leads and the BIDS community is really important for them to succeed.

@francopestilli
Copy link
Collaborator

@JAgho @oesteban if possible we would love to stay informed because of the work we have done before and plan to do later @arokem @mattcieslak @Lestropie @jchoude

@oesteban
Copy link
Collaborator

Okay, I have added one line about the "effective" vs "requested" gradients in (see 487032c).

Comment on lines 526 to 539
The gradient strength and orientation information corresponding to a DWI acquisition
is REQUIRED, and therefore, MUST be stored in the structure.
The gradient strength and orientation information stored in the structure MUST correspond
to "*effective*" values (as in, the actual orientations and strengths applied by the scanner),
rather than the "*requested*" values (as in, the original gradient table design that is
configured into the scanner's MR protocol settings).

The RECOMMENDED way to store gradient information is a `[*_]dwi.tsv` file corresponding
to one or more DWI files.
The format of this TSV file, and associated metadata, are described below.
The gradient information MAY be stored using the `[*_]dwi.bval` and `[*_]dwi.bvec` pair
of files.
However, the `[*_]dwi.[bval|bvec]` files are DEPRECATED and will be
phased out in a future release.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a breaking change shouldn't this be under the 2.x consideration? if not perhaps the wording at present could be changed to support both the current required bval/bvec and add the tsv file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this PR more as a bugfix (and hence that takes precedence over the backward compatibility principle). Happy to see proposals on better wording though.

That said, with the current text both would be supported. TSV would take precedence over bval/bvec, and implicitly (but not explicitly), the tsv should be equivalent to bval/bvec. The only "breaking" change is lifting the mandate to have bvec/bval files (if you have the tsv)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satra did my reply address your concern? How would you address this issue?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry too many things going on. my suggestion would be to continue to require bval and bvec till bids 2.0. regarding it being a bug. is it really a bug, or just that it forces a certain consideration that does not work in all situations. i.e. software have to be aware of the details of the spec. if the latter, that to me is not a bug but an alienation problem, which can be addressed by making the tsv required/recommended moving forward. it's possible i'm not completely understanding the bug part of the spec.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oesteban I am inclined on giving precedence to bvecs/bvals reason being that if we give precedence to TSV only a very narrow set of mainstream toolboxes will be able to handle BIDS DWI. @arokem @jchoude

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW - how does this PR breaks backward compatibility? The deprecation is exactly to tolerate the utilization of the old feature while issuing a strong recommendation of the replacement (https://en.wikipedia.org/wiki/Deprecation) - precisely to avoid breaking changes.

I'd agree with removing the .bvec/.bval only the 2.0 release (which would indeed be the breaking change).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a breaking change in the specification is one where existing tooling stops working as a result of the change. typically deletions of required elements in any schema are breaking changes. additions are also breaking changes if the new keys introduced are required.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a breaking change in the specification is one where existing tooling stops working as a result of the change.

Yup, and since .bval/.bvec will still be allowed, a dataset that currently works will continue working on the same tool. The requirement is to have at least one of the two options (.tsv or .bval/.bvec) - so if the .tsv is not present, the specification remains exactly the same.
The new format is RECOMMENDED, and will only be mandatory when the deprecation finishes. Possibly BIDS 2.0.

The only effect of this PR is that now a BIDS dataset can be written with a much better, unambiguous format for the gradients. And that's not breaking compatibility because scanner coordinates could not be encoded (without conversion to voxels) before.

Anyhow, besides I personally think this is not breaking compatibility, there's also the fact that we indeed discussed how this deprecation would happen when Matt first submitted the PR. It would be unfortunate that we revisited those decisions (for the reasons above, and because that will set some precedent as to maybe preempt future contributions).

My impression is that although this proposal has important substance changes (I particularly like @jchoude's summary above), we seem to be focusing more on the formal aspect of the deprecation (which has largely been discussed already).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i completely agree that this sub discussion has to do with principles of BIDS rather than the substance of this PR. however, if this PR is merged software that worked prior to this could stop working. for example, wouldn't this mean that after this change none of the scripts using the FSL tools will work on new datasets unless someone did an appropriate conversion?

most software doesn't check on bids version for a dataset given that bids has not released 2.0. i think that's the fundamental question in this thread. this PR just happens to be an instance of something that brings this up. i don't want to hold up this PR, but it seems that there are other instances where such an issue could come into play. as bids expands this could happen at a modality specific level such as this, or the common principles level (there were a few inheritance issues that were pushed off to bids 2.0).

thus this is a meta discussion for bids as a whole, not just this PR. but merging this PR, means we have accepted this route at least for one instance.

Copy link
Collaborator

@oesteban oesteban Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for example, wouldn't this mean that after this change none of the scripts using the FSL tools will work on new datasets unless someone did an appropriate conversion?

For existing datasets, not at all - this PR does not mandate the update from bvecs/bvals to tsv.
For future datasets, you can generate the bvecs/bvals from the tsv file. Making this step more explicit will make it harder for the user, but will also ensure data is preserved as it was generated by the scanner (in the tsv file).

If someone is motivated and has the knowledge to implement a test within the BIDS validator to ensure that the tsv and bvec/bval are consistent, that would be really helpful for the DWI community.

(corresponding to the *N* volumes in the corresponding NIfTI file.)
The first row contains the *x* elements, the second row contains the *y* elements and
the third row contains the *z* elements of a unit vector in the direction of the applied
diffusion gradient, where the *i*-th elements in each row correspond together to
the *i*-th volume, with `[0,0,0]` for *non-diffusion-weighted* (also called *b*=0 or *low-b*)
volumes.
Following the FSL format for the `[*_]dwi.bvec` specification, the coordinate system of
Following the FSL format for the `.bvec` specification, the coordinate system of
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The directions in the bvec aren't always using the image axis coordinate system. @frankyeh recently updated DSI Studio to handle the case where the image axes aren't in the FSL "RPI" orientation but the bvecs are. FSL interprets bvecs differently depending on the orientation of the image axis.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I didn't know that. I think this just reinforces the argument that this PR is necessary. How the (recently added) conversion example would change with this? Should we mention the image axis coordinate system so that what is said is fully accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conversion example will only work if the image is in LAS+ orientation. This will be true by default if dcm2niix was used, but is not guaranteed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is the actualFSL spec: https://fsl.fmrib.ox.ac.uk/fsl/fslwiki/FDT/FAQ#What_conventions_do_the_bvecs_use.3F (it should be included in that section)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@francopestilli
Copy link
Collaborator

francopestilli commented Apr 29, 2021

I follow up on @satra

The PR seems to stem from the learning and implementation of new code (I assume). The new code might have better features than the older code (e.g., coordinate system). Yet, as it is new code, it is not embraced by the community yet, it is not known yet and it has not involved the community yet.

So this is a good general discussion. How do we propose changes to the specification that would accommodate new code and general improvements when the code is not out yet, the code has the need been tested either by the BIDS contributor not by the community at large.

Good food for thought I hope.

@effigies
Copy link
Collaborator

Is it always possible to take a new-style gradient TSV and input images and generate a correct set of FSL-style bval/bvecs? If so, then making a small tool that does that might be the cleanest option. bval/bvec can remain REQUIRED which means that old tools that take the standard at its word and assume the files can be found, while users can start with an authoritative TSV file.

We have DEPRECATED things in recent releases, but we have not previously deprecated REQUIRED data. Probably the closest is _phase.nii.gz is now _part-phase_bold.nii.gz and _PD.nii.gz should now specify _PDw.nii.gz or _PDmap.nii.gz. Beyond not being REQUIRED, these are also relatively rare.

We may not be at voting stage yet, but I think I would probably be inclined to say that bval/bvec remain REQUIRED.

@effigies
Copy link
Collaborator

If it's impracticable to validate consistency, we could say that, for tools that accept either TSV or bval/bvec, the TSV is to be considered authoritative.

@mattcieslak
Copy link
Contributor Author

Is it always possible to take a new-style gradient TSV and input images and generate a correct set of FSL-style bval/bvecs? If so, then making a small tool that does that might be the cleanest option. bval/bvec can remain REQUIRED which means that old tools that take the standard at its word and assume the files can be found, while users can start with an authoritative TSV file.

It wouldn't be hard to make a small tool to translate between bval/bvec files and TSVs. MRtrix3 already has a really good one that even handles oblique acquisitions.

It also might not be totally correct to say that there is already a lot of software support for the bvec format. MRtrix (uses the strict FSL spec) and DIPY (uses image axes) handle them differently. DSI Studio has used handled them both ways at different times. DSI Studio, MRtrix and AFNI all have tools that permute the signs of the bvecs to figure out empirically how to interpret them

@francopestilli
Copy link
Collaborator

@mattcieslak I understand the complexity in handling coordinate systems, please do not het ,e started :-) ... yet all of these tools you mention read in bvecs, correct?

@effigies I would say the mapping is lossless this way bvecs -> TSV (but it requires more than just BVECS to know the coordinate system) and is lossy this way TSV -> BVECS (as it loses the coordinate system). Does this help?

@SyamGadde
Copy link

If I may chime in on this very important, long-due improvement:

  1. Recommending that the gradient directions are stored as provided by the scanner (either scanner/realworld-space or image-space) is nice, but implicit in that text is an assumption that any scanner which stores the directions in scanner space will store them in RAS+. But with DICOM, for example, that is clearly not the case (LPS+), and so this recommendation would be impossible for most data. Maybe de-emphasize the "original coordinate system" recommendation and push for "closest coordinate system", since they should look pretty similar aside from some negative signs.
  2. The conversion between .bvec/.bval and .tsv is complicated by the fact that it is dependent on the orientation of the input data, which was one of the major inspirations for this whole issue (i.e Incomplete description of DWI bvecs format #243) In some cases the conversion between bval/bvec and .tsv is not quite as simple as it is currently described because it would need to take the image orientation (and whether it was a rotation of LAS+ or a rotation of RAS+). Pointing to the DTIFIT page doesn't do justice to the danger because most of the FSL pages assume the data is in some rotation of LAS+, and only in a few places do they mention the orientation assumptions. It's probably not worth including support for that in the text, but a link to other documentation (like the mrtrix forum page mentioned in Incomplete description of DWI bvecs format #243) or to a separate discussion page or even just a DANGER sign? The bvec/bval and .tsv alternatives are not equivalent and there is no successful conversion between them without knowing the orientation of the associated image.

Thanks for your work on this!
(USENET group alt.bvec_bvals.die.die.die)

@oesteban
Copy link
Collaborator

oesteban commented Jul 14, 2021

If I may chime in on this very important, long-due improvement:

Of course! Thanks for doing so.

  1. But with DICOM, for example, that is clearly not the case (LPS+)

I may have this wrong, but LPS+ determines the orientation of the voxel cartesian space, while the physical (world) coordinates system is right-handed and defined w.r.t. the scanner's bore, right?.

I would agree though that the proposed header (R, A, S) is unfortunate because actually confuses the image's cartesian grid system with the world system. It would be better to just say X, Y, Z (as opposed to I, J, K that is proposed for voxel coordinates).

  1. The conversion between .bvec/.bval and .tsv is complicated by the fact that it is dependent on the orientation of the input data, which was one of the major inspirations for this whole issue [...]

Yes, this is probably one of the weakest points of this PR. Please send suggestions to clarify this.

@SyamGadde
Copy link

I may have this wrong, but LPS+ determines the orientation of the voxel cartesian space, while the physical (world) coordinates system is right-handed and defined w.r.t. the scanner's bore, right?.

I would agree though that the proposed header (R, A, S) is unfortunate because actually confuses the image's cartesian grid system with the world system. It would be better to just say X, Y, Z (as opposed to I, J, K that is proposed for voxel coordinates).

Sorry I was probably mixing terminology/conventions and not as familiar with the BIDS conventions. So to clarify my point -- independent of the matrix that defines the locations of voxels in space (which could be described in shorthand as RAS+, ALS+, RPI+, etc. where the letters indicate the rough direction in which higher-indexed voxels go), there is the coordinate system defining what the coordinate (x, y, z) means and the convention in NIFTI is where positive numbers mean R, A, and S respectively, which I would also find useful to describe as RAS+ (as opposed to DICOM's LPS+). I actually like that the columns would use R, A, and S as the convention is then explicit in the file. The alternative would be to embed the transformation from gradient directions into world-space (what NRRD would call the "measurement frame") but that would diverge with the "image space" option (with I, J, K columns) and so I think the current approach is simple enough and easy to understand.

  1. The conversion between .bvec/.bval and .tsv is complicated by the fact that it is dependent on the orientation of the input data, which was one of the major inspirations for this whole issue [...]

Yes, this is probably one of the weakest points of this PR. Please send suggestions to clarify this.

I'll try to put some text together. Thanks!

@oesteban
Copy link
Collaborator

oesteban commented Sep 8, 2021

@SyamGadde any chance you find a slot to put together the edits?

@SyamGadde
Copy link

Can you tell me where to target the changes? I was having trouble figuring out how to put together a pull request for this issue and I sent one to the original branch:

mattcieslak#2

Happy to resubmit, just need to know where...

@oesteban
Copy link
Collaborator

oesteban commented Sep 8, 2021

Sorry I missed that. I'll go through it tomorrow

@Lestropie
Copy link
Collaborator

Coming here for the first time due to bids-standard/bids-bep016#26; apologies for missing the discussion to this point after having initially lodged #243, GitHub notifications weren't making it to my inbox.

  1. Unless otherwise controlled, when writing to a NIfTI / JSON, MRtrix3 will actually already write the diffusion gradient table in RASB format to the JSON under the field "dw_scheme". This is an MRtrix key that well predates BIDS. This behaviour results from the fact that in MRtrix we have been able to store sidecar data within image headers for a long time (documentation) (albeit in a string-string key-value list only), including the diffusion gradient table (documentation), and in order to provide the baseline functionality for BIDS support we essentially interpret JSON as being an alternative location for the image sidecar information to be stored rather than the image header. So if that information is present internally within MRtrix3, it will be written to the JSON in our internal representation.

  2. The current proposed format for such data:

    R           A          S          b
    0           0          0          0
    1.0         0          0          1000
    -0.0509541  0.0617551  -0.99679   1000
    ....
    

    will not work out-of-the-box in MRtrix3. We expect anything not commented out to be numerical data. We don't treat files with .b or .tsv file extensions any differently to e.g. .csv, it all goes to a generic numerical matrix parser. It would be possible technically to support this, by permitting only the first non-comment row to be non-numeric; that's something that would need to happen at the MRtrix3 side, just flagging that right now this won't work in MRtrix3-land.

  3. From the PR:

    The gradient strength and orientation information MUST correspond to "effective" values (as in, the actual orientations and strengths applied by the scanner), rather than the "requested" values (as in, the original gradient table design that is configured into the scanner's MR protocol settings).

    There's a couple of different ways this could be interpreted:

    1. Many will know that if you request a b=3000 acquisition, you can get a DICOM where the individual volumes have their b-values encoded as e.g. 2985-3015 or so, as the b-vector is re-calculated based on the full set of gradient waveforms. These are typically however very minor differences, and instead interpreting the data based on the requested gradient table would most likely not be deleterious to any analysis; so imposing a "MUST" is unnecessarily stringent.
    2. If the concern here is instead of the scanner hardware executing the gradient table using its XYZ gradients, but rotating the gradients according to the image FoV, then I agree this is a concern, however the statement is not accurate enough due to the ambiguity with point 1 above.

    The RECOMMENDED .tsv file and the OPTIONAL .bval/.bvec MAY coexist in the structure.

    If these are not to be made mutually exclusive, then I would encourage App developers to think about how their software should behave in the case where both sources of data are provided, but are different to one another (as interpreted by their App). We have struggled with this for many years in MRtrix3 with NIfTI's qform / sform (since we support many image formats other than NIfTI, so expect just a single image transform). Our solution is that if both are specified, we read and interpret both, and if the difference between them exceeds expected precision, we issue a warning, and notify the user as to which is being used (the behaviour of which can be controlled by the user via a config file). Note that such mismatches are wholly likely to occur given the bvec i-flipping issue.

    Each row of the TSV file corresponds to one sampled DWI

    Would suggest "one DWI volume"; don't think I've ever seen the wards "sampled" and "DWI" alongside one another.

    Rows corresponding to b = 0 s/mm2 strengths MUST contain four zeros.

    This to me is unnecessarily stringent. It's entirely common for DWI data in DICOM format to have a non-zero vector direction (and indeed even a non-zero b-value; see 3.1 above). So this places an elevated demand on conversion software to get from DICOM to BIDS. E.g. Using MRtrix3's mrconvert manually to go from DICOM to NIfTI / JSON will currently violate this demand. To me it is up to software to take the data as they are received from the scanner (with minimal conversion) and interpret it as it wishes; if this includes taking volumes with "trivially small" b-values and interpreting them as b=0 and ignoring vector directions, that is the choice of the software developers.

    It is RECOMMENDED to maintain the column ordering as described above (thus, either I J K b
    or R A S b).

    Permitting column permutation here is to me unnecessary. It would also currently break MRtrix3 support. Again, we could implement code there that would read the header line and permute accordingly, but because the file is initially read by a generic numerical matrix read function, it's non-trivial to support. It's a lot of demand on developers to provide flexibility that serves little to not benefit to the end-user.

  4. I do not like the IJK / RAS distinction for multiple reasons:

    1. FSL bvecs is not IJK: it is IJK if the image is interpreted as neurological, regardless of whether it is in fact neurological or radiological. While one can argue that the contents of a .tsv is independent of the FSL bvecs / bvals format and can therefore do as it pleases, inevitably someone somewhere will mistakenly believe that, since both are stored with respect to image axes, they can simply translate information between the two.

    2. This suggests that, if the first image axis most closely corresponds to subject right, and similarly for second / anterior and third / superior, then IJK and RAS are equivalent; but if the image FoV is anything other than pure axial with no angulation, this will not be the case.

    3. Fundamentally (please correct me @jdtournier if I'm wrong), the gradient directions provided in DICOM, as well as the way in which they are interpreted in MRtrix3 is not RAS. It is XYZ: the axes of the scanner hardware. If the subject is in a supine position, then positive-X will be closest to subject right, similarly for positive-Y / subject anterior and positive-Z / subject superior. In BEP016 (Diffusion models) I currently have field "ReferenceAxes" as either "ijk" or "xyz" (though I'm yet to investigate whether or not there are instances in which the FSL bvecs i-flip issue can potentially apply to the outputs of diffusion models also...), names as such for this reason.
      (Edit: @oesteban beat me to it)

  5. Regarding Chris' comments RE rollout strategy:

    1. If .tsv were to truly supersede .bvec / .bval, this would IMO need to happen as part of a major update (i.e. BIDS 2.0) rather than a minor one (e.g. 1.7.0). I'd personally be happy for things to go in this direction. But even then, any BIDS App developer wrapping DWI tools would need to be aware of the issues around representation of this information.

    2. If .tsv is implemented as a non-mutually-exclusive option alongside .bvec / .bval in a minor update, then there's a more general comment about compatibility between BIDS spec version and BIDS Apps that needs to be made (I'm unable to keep up with all BIDS-related discussions, so apologies if there's already some established principle here). If an App is written to (as best as possible given finite development resources) support BIDS data of a particular spec version, but is provided with BIDS data of a newer version, it may or may not work, depending on what changed in the spec. So maybe there needs to be a consensus about how Apps should be producing some form of warning about potential incompatibilities.
      (And this isn't even getting into the massive discrepancy between the amount of effort necessary for an App to support an exemplar BIDS dataset of a particular version, and supporting all possible BIDS datasets that conform to a particular spec version...)

  6. From @effigies:

    Is it always possible to take a new-style gradient TSV and input images and generate a correct set of FSL-style bval/bvecs?

    Yes. MRtrix3 can import & export both .bvec / .bval and what's being called "RAS+b" here (which is also its internal representation); it's all based on the image transformation. It's just easy to get wrong (and we suspect many DWI software packages still do). See e.g. our load and save code.

@jdtournier
Copy link

Just to quickly clarify a statement from @Lestropie - since he saw fit to tag me in...

the gradient directions provided in DICOM, as well as the way in which they are interpreted in MRtrix3 is not RAS. It is XYZ: the axes of the scanner hardware. If the subject is in a supine position, then positive-X will be closest to subject right, similarly for positive-Y / subject anterior and positive-Z / subject superior

Yes and no. First off, apologies if I'm being a bit pedantic or telling everyone what they already know... In the official DICOM specs, and for Philips and Siemens, the gradient vectors are indeed in XYZ, which corresponds to the axes of the scanner hardware in the patient-centered coordinate system (PCS) (official docs here)¹. This only corresponds to the actual hardware coordinate system if patients are positioned head-first supine, otherwise it will be some permutation of these axes (this depends on what the radiographer specifies in the positioning field when registering the patient). But in general, this actually simplifies things, since we can always rely on the assumption that (in DICOM) X is R→L, Y is A→P, and Z is I→S (i.e. LPS+, as others have said). In practice, I expect this is what most people mean when talking about the XYZ coordinate system, since we overwhelmingly deal with HFS positioning (certainly true in my case) - but in my opinion it's more accurately described as RAS+ (though I acknowledge the ambiguity that remains concerning whether we mean scanner or image axes - I mean scanner axes here).

Otherwise, I would echo what others have said elsewhere:

  • Important to have a single spec, and that this be independent of the images. Sticking with the same coordinate system as NIfTI is the most sensible, so I think RAS+ (in patient-centred coordinate system) is the way to go. Not a fan of the IJK option.
  • If you need to have a header line in the file, can we make it a comment? I.e. prefix with a #? If we have a single unambiguous spec, this is probably redundant - though nice to include for completeness. This would make our lives (in MRtrix) a bit easier...
  • The conversion from DICOM shouldn't try to impose too many assumptions. Given that manufacturers tend to store the actual b-value as computed using all imaging gradients, forcing b=0 DW volumes to have a strict zero is likely to backfire for e.g. IVIM acquisitions, which will be acquiring very low b-value images. Best to leave the decision as to what is to be interpreted as b=0 to the end application.

One final point which I don't think has been raised so far is how to handle acquisitions where the magnitude of the gradient vector is modulated to achieve lower b-values than nominally requested - this is to allow multi-shell acquisitions on systems that would otherwise not easily support this (what we call b-value scaling in MRtrix). I may have missed it, but has there been any discussion as to how this should be handled in the spec - if at all?


¹As a side note: before the official DICOM standard, GE would store their gradients in the PRS frame.

Comment on lines +771 to +772
"BigDelta": 0.005,
"SmallDelta": 0.0005
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Given that these appear in a general sidecar metadata space rather than a DWI-specific context, the names "BigDelta" and "SmallDelta" may not be specific enough.

  • If revisiting, it would be worth considering whether it is worthwhile adding these, or instead deferring more advanced diffusion encoding information entirely to the aDWI extension.

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.