-
Notifications
You must be signed in to change notification settings - Fork 169
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
base: master
Are you sure you want to change the base?
Changes from 13 commits
535a9da
364b252
0fd7340
24278e8
524e776
3e6964c
3fa1740
0c26f5b
bd2f956
fa194f9
9ee13ff
487032c
ecfe25f
6c73743
dea99be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -531,9 +531,8 @@ distinguish different sets of phase-encoding directions. | |
**Combining multi- and single-band acquisitions**. | ||
The single-band reference image MAY be stored with suffix `sbref` (for example, | ||
`dwi/sub-control01_sbref.nii[.gz]`) as long as the image has no corresponding | ||
[gradient information (`[*_]dwi.bval` and `[*_]dwi.bvec` sidecar files)](#required-gradient-orientation-information) | ||
[gradient information](#required-gradient-strength-and-orientation-information) | ||
to be stored. | ||
|
||
Otherwise, if some gradient information is associated to the single-band diffusion | ||
image and a multi-band diffusion image also exists, the `acq-<label>` key/value pair | ||
MUST be used to distinguish both images. | ||
|
@@ -542,21 +541,78 @@ In such a case, two files could have the following names: | |
The user is free to choose any other label than `singleband` and | ||
`multiband`, as long as they are consistent across subjects and sessions. | ||
|
||
### REQUIRED gradient orientation information | ||
|
||
The REQUIRED gradient orientation information corresponding to a DWI acquisition | ||
MUST be stored using `[*_]dwi.bval` and `[*_]dwi.bvec` pairs of files. | ||
The `[*_]dwi.bval` and `[*_]dwi.bvec` files MAY be saved on any level of the directory structure | ||
### REQUIRED gradient strength and orientation information | ||
|
||
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. | ||
The TSV file format is OPTIONAL, and will replace `[*_]dwi.[bval|bvec]` as the | ||
default method for representing gradients. | ||
|
||
The `[*_]dwi.[bval|bvec|tsv]` files MAY be saved on any level of the directory structure | ||
and thus define those values for all sessions and/or subjects in one place (see | ||
[the inheritance principle](../02-common-principles.md#the-inheritance-principle)). | ||
|
||
As an exception to the [common principles](../02-common-principles.md#definitions) | ||
that parameters are constant across runs, the gradient table information (stored | ||
within the `[*_]dwi.bval` and `[*_]dwi.bvec` files) MAY change across DWI runs. | ||
within `[*_]dwi.[bval|bvec|tsv]` files) MAY change across DWI runs. | ||
|
||
**Gradient table specification (`_dwi.tsv` file)**. | ||
Gradient strength and orientation can be provided in a TSV file named, for example, | ||
`sub-01_acq-singleband_dwi.tsv` or `sub-01_acq-multiband_dwi.tsv`. | ||
This TSV file must contain four columns, where the first three columns correspond to the | ||
three components of a unit-norm vector determining one gradient orientation (also called "*b*-vector") | ||
and the last column contains the gradient strength ("*b*-value") in s/mm<sup>2</sup>. | ||
Each row of the TSV file corresponds to one sampled DWI, and therefore, the number of rows | ||
MUST match the number of volumes in the corresponding DWI file(s). | ||
In other words, the number of rows in the TSV file and the size of the last dimension of the | ||
data array in the corresponding DWI file(s) MUST be the same. | ||
|
||
Rows corresponding to *b = 0* s/mm<sup>2</sup> strengths MUST contain four zeros. | ||
Correspondingly, the components of the *b*-vector associated with a nonzero gradient strength | ||
(*b* > 0) MUST represent a unit-norm vector. | ||
|
||
Scanner devices may store the *b*-vectors with reference to the acquired voxel or the | ||
scanner coordinate system (see [Coordinate systems](../99-appendices/08-coordinate-systems.md)). | ||
The coordinate system specification is REQUIRED, and MUST be encoded by the column headers | ||
of the TSV file. | ||
Therefore, if the *b*-vectors in the gradient table are given in *voxel coordinates*, | ||
the column headers MUST be `I`, `J`, `K`, `b`. | ||
We will refer to this format as *IJK+b* in the following. | ||
Alternatively, if the *b*-vectors in the gradient table are given in *scanner coordinates*, | ||
the column headers MUST be `R`, `A`, `S`, `b`. | ||
We will refer to this format as *RAS+b* in the following. | ||
|
||
It is RECOMMENDED to store the gradient table information in the original coordinate system | ||
stored by the scanner (therefore, unchanged). | ||
|
||
It is RECOMMENDED to maintain the column ordering as described above (thus, either `I J K b` | ||
or `R A S b`). | ||
|
||
Example of `_dwi.tsv` file, following the *RAS+b* convention: | ||
|
||
```Text | ||
R A S b | ||
0 0 0 0 | ||
1.0 0 0 1000 | ||
-0.0509541 0.0617551 -0.99679 1000 | ||
``` | ||
|
||
**Gradient orientation file formats**. | ||
**Legacy `.bvec` & `.bval` specification of gradient information**. | ||
The `[*_]dwi.bval` and `[*_]dwi.bvec` files MUST follow the | ||
[FSL format](https://fsl.fmrib.ox.ac.uk/fsl/fslwiki/FDT/UserGuide#DTIFIT): | ||
[FSL format](https://fsl.fmrib.ox.ac.uk/fsl/fslwiki/FDT/UserGuide#DTIFIT). | ||
|
||
The `[*_]dwi.bvec` file contains 3 rows with *N* space-delimited floating-point numbers | ||
(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 | ||
|
@@ -665,19 +721,36 @@ sub-<label>/[ses-<label>/] # MultipartID | |
|
||
### Other RECOMMENDED metadata | ||
|
||
#### Phase-encoding information (common metadata fields) | ||
|
||
The `PhaseEncodingDirection` and `TotalReadoutTime` metadata | ||
fields are RECOMMENDED to enable the correction of geometrical distortions | ||
with [fieldmap information](#fieldmap-data). | ||
See [Common metadata fields](#common-metadata-fields) for a list of | ||
additional terms that can be included in the corresponding JSON file. | ||
|
||
#### Pulse gradient timing information | ||
|
||
Although the *b*-value is the most common way to convey gradient strength, it does not | ||
unambiguously depict the sampled coordinate in *q*-space. | ||
The relationship between each *b*-vector and its corresponding *q*-space coordinate | ||
can be calculated with the `BigDelta` and `SmallDelta` metadata fields in the corresponding | ||
JSON file. | ||
|
||
| Field name | Definition | | ||
| :------------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | ||
| SmallDelta | RECOMMENDED. Acquisition pulse separation time (in seconds). For Siemens, this is in the shadow header (0029,1120) under `sSpecPara.dSpecLipidSupprBandwidth` in milliseconds. This value is necessary for estimating ensemble average propagators in physical units. | | ||
| BigDelta | RECOMMENDED. Acquisition pulse separation time (in seconds). For Siemens, this is in the shadow header (0029,1120) under `sSpecPara.dSpecLipidSupprDeltaPos` in milliseconds. This value is also necessary for estimating ensemble average propagators in physical units. | | ||
|
||
JSON example: | ||
|
||
```JSON | ||
{ | ||
"PhaseEncodingDirection": "j-", | ||
"TotalReadoutTime": 0.095, | ||
"B0FieldSource": ["phasediff_fmap0", "pepolar_fmap0"] | ||
"B0FieldSource": ["phasediff_fmap0", "pepolar_fmap0"], | ||
"BigDelta": 0.005, | ||
"SmallDelta": 0.0005 | ||
oesteban marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+771
to
+772
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
``` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
- .nii.gz | ||
- .nii | ||
- .json | ||
- .tsv | ||
- .bvec | ||
- .bval | ||
entities: | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.