[ENH] Restructuring of DWI derivatives #5
Conversation
|
Hi @Lestropie thanks for this. I will look over it ASAP. I am going to ping also @arokem to see if he has time and make sure what we decide is also compatible with Dipy. |
…fields from BEP16 Mapped from @Lestropie's bids-standard/bids-bep016#5 Closes bids-standard/bids-bep016#2 I have removed some keys that I believe should remain in BEP16 for being applicable to dwi only. I have also changed _FieldInhomogeneity*_ with _SusceptibilityDistortion*_ since I think the latter will be more familiar to a wider audience.
|
Hi @bids-standard/derivatives-mri-dwi ! any chance you can have a look before tomorrow's meeting? |
|
Hi @oesteban sorry for the delay. I was swamped with work for quite a big contract in my day job, so didn't have that much free time to check this. I'll go through it in the coming hours. |
| | 7 | *l* = 4, *m* = 3 (imaginary part) | | ||
| | ... | etc. | | ||
|
|
||
| - Normalisation: ***TODO*** |
There was a problem hiding this comment.
@Lestropie : should this support both mrtrix 2.0 style normalization and mrtrix 3.0 style normalization? How do we describe these two conventions?
Do I understand correctly that this is the issue here?
There was a problem hiding this comment.
Decided to leave out the MRtrix 0.2 convention; that software is likely almost never used, and the orthonormal basis in MRtrix3 has been the default for many years now. Indeed we're considering removing support for the old non-orthonormal basis entirely in MRtrix3.
What this statement more refers to is that the spec requires a description regarding how the overall magnitude of the SH coefficients should be interpreted. E.g. If the l=0 term is 1.0 and all other coefficients are 0.0, does this mean that the FOD amplitude is 1.0 in all directions? Or that the integral over the half-sphere / full-sphere is 1.0? E.g. see: Wikipedia.
I got Donald to revise his documentation around this standard in MRtrix3/mrtrix3#1635; so I will look to update this spec accordingly.
There was a problem hiding this comment.
@Lestropie did you finally update the PR accordingly?
There was a problem hiding this comment.
Moved this to #8, which currently has the branch of this PR as the target.
There was a problem hiding this comment.
Having said that, I put that in a separate PR in order to provide a clean online diff, but it seems to be off somehow. I'll have to figure out what's happened with the force pushes to dwi_derivatives_restructure.
|
There's an outstanding issue with respect to the naming of pre-processed DWI data. While I'm not following the more general BIDS Derivatives discussion as closely as I perhaps need to, I seem to recall that the consensus is that no derivatives file should be named in such a way that it would be a valid file in a raw BIDS dataset. If this is the case, the pre-processed DWI data will need to be somehow named differently. (Personally I don't agree entirely with this: the fact that the file is stored within a derivatives directory should be adequate to identify it as a derivative file; but thems be the rules) |
Based on suggestion in #5. If all model intrinsic parameters are incorporated into a single file, rather than dropping the "_parameter-<param>" field, instead enforce that parameter name "all" be used.
Based on suggestion in #5. If all model intrinsic parameters are incorporated into a single file, rather than dropping the "_parameter-<param>" field, instead enforce that parameter name "all" be used.
oesteban
force-pushed
the
dwi_derivatives_restructure
branch
from
8b27828 to
7ab67ce
Compare
This PR splits the tractography section from the diffusion derivatives document, so that #5 is easier to merge. The new ``05-diffusion-derivatives-tractography.md`` file will remain orphaned, but kept there as a base for the time we tackle tractography. It shouldn't be merged into the derivatives branch until it is ready.
Based on suggestion in #5. If all model intrinsic parameters are incorporated into a single file, rather than dropping the "_parameter-<param>" field, instead enforce that parameter name "all" be used.
oesteban
force-pushed
the
dwi_derivatives_restructure
branch
from
7ab67ce to
37400ef
Compare
- More clarity of distinction between requisite and optional files in output directory. - Try using 3 spaces rather than 4 in non-code indentation; partly to try to get tables within dot point lists to render correctly, partly to improve editor software syntax highlighting.
- Various small re-wordings. - Slightly more use of hyperlinks. - Short introductions to "parameter terminology" and "data representations" sections. - Be more explicit about normalised vs. non-normalised 3-vectors, so that structure more clusely mimics that of description of spherical coordinate representation. - Rename hyperlink names to "parameter terminology" section to better separate from later "intrinsic / extrinsic model parameters" sections.
Based on suggestion in #5. If all model intrinsic parameters are incorporated into a single file, rather than dropping the "_parameter-<param>" field, instead enforce that parameter name "all" be used.
When introducing the file naming convention, give an example of the "_<model>" field.
Re-arranged descriptions of intrinsic and extrinsic model parameters within the file naming section, and corrected a discordance in one dot point that was using an intrinsic model parameter filename path but discussing extrinsic model parameters.
Provide information on specification of orientation data after the various models and model parameters have been explained.
oesteban
force-pushed
the
dwi_derivatives_restructure
branch
from
37400ef to
a804aeb
Compare
|
Hi @Lestropie, I'm on hold to see whether #5 (comment), #5 (comment), and #5 (comment) have been addressed or I need to open new issues for them. After clarifying that, I think we should merge this in. |
|
Left comments on the relevant threads. In general I don't think we should try to be too careful with this particular PR as though it represents the final version; the main purpose of it was the restructuring. Individual nuanced issues like these can always be distributed across individual issues / PRs, and it will make it all easier to track. |
Am not making much progress on #4 after an initial burst, so I'm posting what I have thus far.
There's going to be a lot of discussion required on a lot of different things, so in some instances it may be useful to actually break discussions off into separate GitHub issues. But I'll create an initial list here.
Pre-processed data
File naming convention
Data representations
Orientation specification
Does FSL assume a neurological coordinate system for 3-vectors like it does for bvecs (e.g. see Incomplete description of DWI bvecs format bids-specification#243)? If so, "
ReferenceAxes" needs to be revised.Revision of the SH basis description in MRtrix3 is currently undergoing debate (see SH description: Revise MRtrix3/mrtrix3#1635); contents will likely be updated here correspondingly.
This includes a good description of normalisation of the basis, and the orthonormality of the basis, which are currently missing.
Description of the "
Descoteaux" SH basis is currently absent. I believe it's something about negating coefficients for SH degrees not divisible by 4?Diffusion models
As a high-level comment on models: Rather than trying to make this list as long as possible straight away, I would instead advocate going the other way: make this list fairly short, create individual issues / pull requests for individual models, and attempt to engage software developers / community users in deriving appropriate definitions of each model. Ideally any model that appears within the spec would have had at least one person somewhere actually try to convert data output by some software into a format compatible with the spec, and for that process to have provided some information toward the definition of the spec.
Diffusion model list
Model list currently omits dRL. I believe the output is in the "Amplitudes" format, but I've not found any software for applying such.
Model list currently omits GQSI
Current naming of
noddimay preclude future addition of Bingham-NODDI. Should we modify accordingly / introduce both into the spec?Also unsure as to the standard reference axes for NODDI output
WMTI proposal does appear to conflict quite a bit with the software source I looked at (https://github.com/NYU-DiffusionMRI/Diffusion-Kurtosis-Imaging). This is a good example where if there is one "principal" piece of software for fitting a particular model, then the format of the data generated by that software may inform the specification, as long as it doesn't conflict severely with the "spirit" of the spec.
Currently running
bedpostxwith the different model options; will revise the spec accordingly to make sure it's not incompatible with use of a non-default setting.While I've changed "bedpostx" to "Ball-and-sticks", it still may not be the best level of granularity: should we in fact instead use some other model name, for a more general "discrete mixture model", of which
bedpostxprovides a small number of specific instances? E.g. Sometimes the extra-cellular component is not a "ball", but a tensor.Q-ball: Confirm that there are both discrete amplitudes and SH-based versions?
Many models require more comprehensive description / citations
Input parameters
Many of the parameter names are too brief / cryptic, and most do not have any description whatsoever.
CSD parameters: Should "
ResponseFunctionTensor" follow dipy convention more closely: a list where the first element is a 3-vector containing eigenvalues, and the second element is the reference b=0 intensity?Extrinsic model parameters
adcbe an acceptable alternative tomd? Many missing elementsPrimary JSON sidecar
Issue with proposed field "
Gradients" in JSON file. Can't be three elements each in "Gradients": what if two different shells have volumes with the same sensitisation orientation, but only one of them was used? Really what's needed here is a complete separatebvecs/bvalscontaining what was used in the model fitting. Personally I'd rather have the utilised gradient table embedded within the JSON (e.g. New function DWI::stash_DW_scheme() MRtrix3/mrtrix3#774); but that wouldn't be consistent with the rest of BIDS, which went withbvecs/bvals...Proposed dictionary "
Parameters" in JSON: Is there any real benefit to having the "Parameters" nested like this? Just having a single layer of key-value pairs, with the ones listed above being reserved, would be much simpler... Alternatively should it be called "ModelParameters" in order to separate from "BootstrapParameters"? (Not sure where I stand on this one)Demonstrative examples
References
Does not appear to be any established standardised format for providing non-inline literature references; these will become more regular as derivatives are introduced. I might post this as an issue on the main bids-specification repository.
Our references need to be provided in complete form.