bids-specification icon indicating copy to clipboard operation
bids-specification copied to clipboard

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

Open mattcieslak opened this issue 5 years ago • 33 comments

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

mattcieslak avatar Oct 20 '19 14:10 mattcieslak

I added a table describing BigDelta and SmallDelta. These address @arokem's suggestion https://github.com/bids-standard/bids-specification/issues/349#issuecomment-544201991. 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

mattcieslak avatar Oct 21 '19 01:10 mattcieslak

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.

oesteban avatar Oct 22 '19 16:10 oesteban

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.

mattcieslak avatar Oct 22 '19 19:10 mattcieslak

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.

jhlegarreta avatar Oct 23 '19 00:10 jhlegarreta

+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.

chrisgorgo avatar Oct 26 '19 17:10 chrisgorgo

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!

jchoude avatar Nov 11 '19 19:11 jchoude

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

oesteban avatar Oct 19 '20 10:10 oesteban

@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?

oesteban avatar Apr 05 '21 07:04 oesteban

@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?

francopestilli avatar Apr 05 '21 14:04 francopestilli

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.

arokem avatar Apr 06 '21 02:04 arokem

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

mattcieslak avatar Apr 06 '21 13:04 mattcieslak

Thanks! Let's get this done then and we'll see how the preprint fits independently.

On Tue, Apr 6, 2021, 15:04 Matt Cieslak @.***> wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bids-standard/bids-specification/pull/352#issuecomment-814103133, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAESDRUVZ72LBISH3WEMIC3THMBGTANCNFSM4JCUTX4A .

oesteban avatar Apr 06 '21 13:04 oesteban

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

francopestilli avatar Apr 06 '21 14:04 francopestilli

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.

JAgho avatar Apr 06 '21 14:04 JAgho

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.

oesteban avatar Apr 07 '21 14:04 oesteban

@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

francopestilli avatar Apr 07 '21 14:04 francopestilli

Rebased after fixing conflicts.

oesteban avatar Apr 21 '21 08:04 oesteban

@mattcieslak I have sent a PR against this branch (mattcieslak/bids-specification#1) with a reorganization following this rationale:

  • I think we can make the TSV format more flexible, and recommend users to write the gradient table as it is provided by the scanners. If the scanner produces scanner coordinates, then the header is RAS+b. If the scanner produces voxel coordinates, then the header is IJK+b.
  • The SmallDelta, BigDelta specification was split into two portions. I've grouped them together under the new subsection you created.
  • ~~I've corrected something about the FSL format that I think was blatantly wrong - everyone should carefully review this edit just in case it's me who is wrong.~~ -> I have opened bids-standard/bids-specification#782 for this, and also I have to say that blatantly wrong is a very unfortunate misrepresentation of the problem.

Matt, if you are fine with my changes, please merge them ASAP so that we can push this PR to the finish line within the next couple of weeks. I think this flexibilization of the TSV format will make things a lot easier for the maintainers and software (e.g. PyBIDS) to digest, as in most of the cases this will only entail transposing the bvec and bval files and adding the IJKb column names.

(EDITED - see scratched out text)

oesteban avatar Apr 21 '21 08:04 oesteban

I'm very interested in @jchoude's opinion after the latest developments:

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.

I think the proposed TSV file (which can be given in voxel or world coordinates) could easily be projected into the Derivatives specs and complete it there with the mentioned metadata.

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.

Hopefully, this is very clearly stated now. Both scanner RAS+ and voxel conventions are supported, and they must be encoded in the TSV file's column headers.

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?

This is the point where I'm most concerned. Is this PR clear about that we are talking of "effective" directions? If you feel it is not, then please add concrete suggestions that would clarify this point.

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.

My impression is that the preprint above addresses this problem, and that the problem is beyond the scope of this PR (which is very operational - remove the limitation of encoding gradients with FSL-style files only).

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.

With the change that the TSV file can accommodate voxel coordinates, transitioning from .bval/.bvec to .tsv is immediate. Tools like PyBIDS could move from .bval/.bvec-centric implementations to table-centric without any overhead (instead of get_bvec and get_bval you may have something like get_btable which internally reads and composes .bvec/.bval if tsv is not available).

oesteban avatar Apr 23 '21 07:04 oesteban

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

oesteban avatar Apr 23 '21 07:04 oesteban

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.

francopestilli avatar Apr 29 '21 15:04 francopestilli

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 avatar Apr 29 '21 15:04 effigies

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.

effigies avatar Apr 29 '21 15:04 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? 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

mattcieslak avatar Apr 30 '21 17:04 mattcieslak

@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?

francopestilli avatar Apr 30 '21 21:04 francopestilli

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 #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 #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)

SyamGadde avatar Jul 13 '21 16:07 SyamGadde

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.

oesteban avatar Jul 14 '21 08:07 oesteban

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!

SyamGadde avatar Jul 14 '21 13:07 SyamGadde

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

oesteban avatar Sep 08 '21 20:09 oesteban

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:

https://github.com/mattcieslak/bids-specification/pull/2

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

SyamGadde avatar Sep 08 '21 20:09 SyamGadde