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] Various proposed changes to diffusion derivatives #205

Merged
merged 11 commits into from May 5, 2019
Merged

[ENH] Various proposed changes to diffusion derivatives #205

merged 11 commits into from May 5, 2019

Conversation

Lestropie
Copy link
Collaborator

A duplicate of chrisgorgo#6 as requested by @chrisgorgo. A large volume of relevant discussion can be found in the original PR; discussion related to finalisation of the proposed changes can be made here.

Following on from ea2bc96.
- Fix grammattical error in description of DWI JSON.
- Modify description of recommended NODDI model output.
- For "fwDTI" model, separate tensor coefficients and free water fraction data into two separate NIfTI images.
- For DTI model, include iterative weighted least squares as an option, and separate RESTORE (outlier rejection) as being a separate parameter rather than a method of fitting.
- For CSD model, offer two mechanisms for specifying the response function; also remove MRtrix 0.2 SH basis, and remove "none" from NonNegativityConstraint (as without a non-negativity constraint, it would not be CSD).
- Provide additional examples for DWI model outputs.
- Remove NODDI parameters from "model-derived maps", as these are re-classified as fundamental model output parameters.
- Remove DECFA from "model-derived maps", and instead define "directionally-encoded colour maps" as a different form of model-derived map, since any scalar map can be augmented with DEC.
- Add "Count" as a compulsory parameter for tractography output.
- For tractography output, provide three distinct dictionaries within the JSON: "Constraints" (ROIs and anatomical tissue constraints), "Parameters" (for parameters specific to the streamlines tractography algorithm), and "Seeding" (since determining streamline seeds is in fact independent of the particular tractography algorithm and parameters). Field "TerminationCriterion" was removed in this process as it is common for there to be more than one mechanism by which streamlines are terminated. Possibilities for streamline seeding have been more broadly generalised as part of this process.
@effigies
Copy link
Collaborator

Just want to call the attention of: @oesteban, @francopestilli, @jchoude, @arokem.

I believe this is one of two outstanding issues on the derivatives BEP.

@jchoude
Copy link
Contributor

jchoude commented Apr 19, 2019

For myself, I'll get on this on Friday night. Sorry for the lag.

Copy link
Contributor

@jchoude jchoude left a comment

Choose a reason for hiding this comment

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

Hi @Lestropie and @effigies thanks for your patience. I went through this and have only small comments (mainly typos).

Thanks for all this work @Lestropie I think this version would be good to merge in the spec. I quickly went back through the old discussions on the previous PR, and everything seems in line with the overall discussions. I don't see anything here that isn't in line with the spirit of the Diffusion derivatives.

Thanks again!

src/05-derivatives/05-diffusion-derivatives.md Outdated Show resolved Hide resolved
| `DKI` | Diffusion kurtosis imaging (Jensen et al., 2005) | 4D image with Dxx, Dxy, Dxz, Dyy, Dyz, Dzz, Wxxxx, Wyyy, Wzzzz, Wxxxy, Wxxxz, Wxyyy, Wyyyz, Wxzzz, Wyzzz, Wxxyy, Wxxzz, Wyyzz, Wxxyz, Wxyyz, Wxyzz; Where D is the diffusion tensor and W is the kurtosis tensor. |
| `WMTI` | White matter tract integrity (Fieremans et al., 2011) | 4D image with Dxx, Dxy, Dxz, Dyy, Dyz, Dzz, Wxxxx, Wyyy, Wzzzz, Wxxxy, Wxxxz, Wxyyy, Wyyyz, Wxzzz, Wyzzz, Wxxyy, Wxxzz, Wyyzz, Wxxyz, Wxyyz, Wxyzz, Dhxx, Dhxy, Dhxz, Dhyy, Dhyz, Dhzz, Drxx, Drxy, Drxz, Dryy, Dryz, Drzz, AWF; where D is the diffusion tensor, W is the kurtosis tensor, AWF is the additional axonal water fraction parameter |
| `CSD` | Constrained Spherical Deconvolution (Tournier et al. 2007; Descoteaux et al. 2009) | 4D image with spherical harmonic (SH) coefficients (number of volumes and their ordering depends on the model maximal degree and basis set, specified in the sidecar) |
| `NODDI` | Neurite Orientation Dispersion and Density Imaging (Zhang et al. 2012, Daducci et al., 2015) | Three 3D images, with <parameter> equal to {`ICVF`,`OD`,`ISOVF`}: ICVF is the “intracellular volume fraction” (also known as NDI); OD is the “orientation dispersion” (the variance of the Bingham; also known as ODI); ISOVF is the isotropic component volume fraction (also known as IVF). Additionally a vector-valued map with the parameter name "`direction`" may provide the XYZ direction of the estimated fibre orientation. |
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the <parameter> tag does not render correctly, at least when using the View file rendering option directly in Github.

src/05-derivatives/05-diffusion-derivatives.md Outdated Show resolved Hide resolved
- `Basis` : {`MRtrix 0.2`,`MRtrix3`,`DESCOTEAUX`}
- `Tissue` : string
- `SphericalHarmonicDegree` : value
- `ResponseFunctionZSH` : values (1 row per unique *b*-value as listed in "`Shells`"; 1 column per even harmonic degree starting from zero)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is from the comments on the previous review (the one targeting Chris's branch), but I agree with the options as defined now.

- `ResponseFunctionOrder` : value
- `ResponseFunction` : \[1:response_function_order_value,b0mean\]
- `Basis` : {`MRtrix 0.2`,`MRtrix3`,`DESCOTEAUX`}
- `Tissue` : string
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a controlled vocabulary, listing the possible tissue types?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hi @jchoude thanks, how would you change this specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if we should have a list of possible tissues and the spelling, like [white|gray|csf], to avoid people writing the same tissue type in various ways , eg: 'white matter', 'WM', 'white', etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd be open to a limited controlled vocabulary; i.e. have pre-defined strings recommended for use, but still permit use of items not in the vocabulary. While MSMT is most commonly done with WM / GM / CSF tissues, it's in no way obliged to use such, and in time there will be an increasing number of use cases that don't conform to such. Is there an appropriate syntax for such in the spec?

Also note that I would generally expect an abbreviated name of the tissue to appear in the file name, probably under the "desc-" tag. So the "Tissue" string in the JSON is in one way redundant; but conversely it does give the opportunity for a more verbose name than what would be crammed into the filename.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, rethinking about it we might not want to add another vocabulary at this point. Let's leave it like that.

src/05-derivatives/05-diffusion-derivatives.md Outdated Show resolved Hide resolved
src/05-derivatives/05-diffusion-derivatives.md Outdated Show resolved Hide resolved
src/05-derivatives/05-diffusion-derivatives.md Outdated Show resolved Hide resolved
@effigies
Copy link
Collaborator

effigies commented Apr 23, 2019 via email

- Minor spelling & grammar fixes.
- In "scalar maps" section, expand out the different parameters to be derived from the ball-and-stick(s) model.
- Provide more accurate link to MRtrix3 tracks file format.
@Lestropie
Copy link
Collaborator Author

Some more updates in 74ff6d0.

One other thing that caught my eye was the labelling of "BedpostX" as a model. Personally I see this as a software command that fits a particular model, with that model being "Ball-and-sticks", with the command then applying bootstrapping over the top of that. It might however be worth getting this PR over and done with and then proposing this change separately.

- Fix use of superscripts in "Scalar maps" table.
- For "Units" field in tractography JSON file, list possible values in the same format as that used elsewhere in the document.
@jchoude
Copy link
Contributor

jchoude commented Apr 26, 2019

I reviewed your new changes and they all address the few last comments.

I agree with you about BedpostX, and, as you said, this should probably be done in a followup PR.

To me, this looks good to go. I currently cannot merge, since I am not a BEP leader, but I would happily do so if that was the case.

Thanks Robert (@Lestropie ) for all this work!

@francopestilli francopestilli merged commit b48d068 into bids-standard:derivatives May 5, 2019
Copy link
Collaborator

@francopestilli francopestilli left a comment

Choose a reason for hiding this comment

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

looks good!

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.

None yet

4 participants