-
Notifications
You must be signed in to change notification settings - Fork 190
[ENH] BEP 16: Diffusion modeling derivatives #2258
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
base: master
Are you sure you want to change the base?
Conversation
* ENH: Restore diffusion derivatives * MAINT: Split out tractography 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. * First commit for restructuring of DWI derivatives * BEP016: Small fixes, and try to get links to headers working * BEP016: More updates of internal header links * BEP016: Minor tweaks from first feedback round - 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. * BEP016: Try to get links working within tables * BEP016: More minor tweaking - 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. * BEP016: Transpose SH volume count tables * BEP016: Provide example for parameter definitions * BEP016: Change to single-file diffusion models 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. * BEP016: Initial explanation of filename When introducing the file naming convention, give an example of the "_<model>" field. * BEP016: File naming clarification 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. * BEP016: Spelling fix * BEP016: Fix missing anchor links * BEP016: Move orientation specification Provide information on specification of orientation data after the various models and model parameters have been explained. * BEP016: Revise SH description Revised based on MRtrix3/mrtrix3#1635. Manual merge / cherry-pick of: c6539851, 8b278282, c6539851, ebbf0d3d, d7b4731d, 99e92e0a, 6bfc7849 due to unresolvable conflict in bids-standard/bids-bep016#8. * BEP016: Initial conformity to markdown formatting * BEP016: More formatting conformity changes * BEP016: More formatting conformity changes * BEP016: Attempted fixes to table cell padding * BEP016: Minor fix to tractography derivatives for CI pass * BEP016: Modify nomenclature around MRtrix3 SH coefficients * BEP016: Revise preprocessed DWI data example - Provide basic instructions rather than elaborating on rare use cases. - Remove JSON dictionary on preprocessing steps utilised as these relate to provenance and are therefore out of scope. Proposal for addressing bids-standard/bids-bep016#25. * BEP016: Remove SH Descoteaux placeholder * BEP016: Remove majority of models Introduction and review of most models are intended to be deferred as per bids-specification/bids-bep016#39. * BEP016: Formatting fix following removal of models * BEP016: Strip out tractography content * Fixes a typo caught by codespell * BEP016: Initial draft of common filename suffix "model" * BEP016: Rename "dti" to "tensor" * Rename "_parameter-" entity to "_param-" * DWI derivatives: Compulsory "parameter" entity While the specification states that the "parameter" entity must always be defined---even if all model parameters are encoded within a single image, in which case value "all" must be used---two of the demonstrative examples did not obey such. * DWI derivatives: Resolution between content changes Resolves bids-standard/bids-bep016#52 (making "parameter" entity compulsory) against bids-standard/bids-bep016#51 (changing key for that parameter from "parameter" to "param"). * DWI derivatives: Fix tensor model example * DWI derivatives: "model" as entity * DWI models: Restore renaming "dti" to "tensor" Changes in #47 were lost in the process of conflict merging. * BEP016: Remove reference to old terminology Formerly known as "intrinsic" model parameters are now (currently) referred to as simply "model parameters". * BEP016: Revert "_param-all" In cases where all model parameters can be encapsulated in a single NIfTI image, revert back to the case where the "param" entity is omitted from the file name. Reverts bids-standard/bids-bep016#52. * BEP016: Adopt "model fit parameters" terminology In order to better disambiguate the various types of "parameters" defined, change "model parameters" to "model fit parameters". * BEP016: Remove references to removed "pdf" data representation * DWI derivatives: Make tensor a data representation * DWI derivatives: Fix ModelDescription in exemplars Specification requests field "ModelDescription", but exemplars at end of document instead erroneously used key "Model". * Adds the descoteaux 2007 convention. Based on https://github.com/dipy/dipy/blob/master/dipy/reconst/shm.py#L427-L430 * Change from strange html encoding to "<" and ">" * Fix codespell-detected British spellings. * Replace the latin "etc" with ellipses. * Bump actions/checkout from 3 to 4 Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> * BEP016: Major changes - Reject suffices "mfp" (or earlier "_model") / "mfp" in favour of "_dwimap". Instead adopt distinction of "model metadata" vs. "parameter metadata". - Reject use of advanced inheritance in favour of listing all relevant metadata for each data file in the sidecar JSON. - Greater use of sub-dictionaries in JSON files to assist in separating metadata relevant to a model as a whole vs. only that particular parameter of the model. * BEP016: Fis JSON formatting in demonstrative examples * Diffusion models: Forbid negative spherical coordinate radius * Diffusion model derivatives: Spelling fixes * Apply US spelling. Addresses current codespell CI failure. * Markdown table linting * Get rid of a few latin phrases (i.e., "e.g."). * DWI models: Define "bvec" orientation reference * DWI models: Force presence of "param" entity * DWI models: add "ParameterURL" metadata field * Clarify metadata fields relevance (#102) * DWI models: Clarify metadata fields relevance * DWI models: Typo fix --------- Co-authored-by: Ariel Rokem <[email protected]> * Removes one more i.e. * BEP016: Utilize filesystem macro * BEP016: Reformat demonstrative examples * BEP016: Don't use filesystem macros for templates * BEP016: Comment filesystem macros * DWI models: Update bvec reference - FSL bedpostx command fibre orientations stored as spherical coordinates are confirmed to use the "bvec" reference frame. - Enforcing azimuth angle to obey the right hand rule about the zenith axis would necessitate direct modification of image intensities from bedpostx outputs to store. Therefore it would be preferable to instead define the sign of the azimuth angle based on the direction of the second reference axis. Closes #95. Closes #94. * Fixes links. * Use naming convention without number prefix. * Use US spelling of "realizations". * Allow "burnin", which is a technical term used in describing ball and stick. * Try to sort out markdown linting. * Remove remaining instances of `_model` suffix. * Adds macros for tables. Adds an example of NODDI fit. * Start converting metadata tables to macros. * Fixes malformed json in table. * More malformed json fixed. * More metadata tables converted with macro. * Fixes malformed json in macro. * Removes backticks in json. * Remove side_car_table call that makes no sense here. * Adds diffusion derivatives to toc. * Fix bval/bvec extensions. * Adding parameters needed to describe a noddi fit. * diso and dpar should be in the Parameters dict of the model. * Fixes key name in model metadata dict. * Use mm-related values, because b-values are already encoded in that scale. This diverges from the usual recommendation to use SI units, but makes sense in this field, where values of mm^2/s are used to describe diffusivity in many different places. * Adds parameterURL for noddi parameters. * Add metadata fields to schema. I didn't use any references so the glossary links will be broken. * Address remark warnings. * Try to fix warnings. * Move comments into GitHub PR discussion. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Christopher J. Markiewicz <[email protected]> Co-authored-by: oesteban <[email protected]> Co-authored-by: Robert Smith <[email protected]> Co-authored-by: Franco Pestilli <[email protected]> Co-authored-by: PeerHerholz <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Salo <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2258 +/- ##
=======================================
Coverage 82.83% 82.83%
=======================================
Files 20 20
Lines 1672 1672
=======================================
Hits 1385 1385
Misses 287 287 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Surfacing some comments that were embedded in the document, but that are probably better addressed here (with small edits, for clarity):
I think that we should leave this as it is now. That is, enforce specific axes.
I don't think that we want a new entity for this. I think that adding indices is fine.
I agree. I think that this should be handled by providing some "canonical" conversion software. @PeerHerholz : does the software that you wrote cover these issues? |
|
@PeerHerholz : there was some discussion of examples on #2211. Have you made any progress on that? |
Also add model and parameter entities to schema. Fix tables.
Sorry, I missed this in the other |
| OrientationEncodingTypeAmplitudes: | ||
| selectors: | ||
| - dataset.dataset_description.DatasetType == "derivative" | ||
| - suffix == "dwimap" | ||
| - sidecar.OrientationEncoding.Type == "amplitudes" | ||
| fields: | ||
| AmplitudesDirections__OrientationEncoding: | ||
| level: required | ||
| level_addendum: if `OrientationEncoding.Type` is `"amplitudes"` |
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 am having trouble figuring out how to validate subobjects' field values. In this case, if "OrientationEncoding"["Type"] is "amplitudes", "OrientationEncoding"["AmplitudesDirections"] MUST be defined.
Introduces specifications for diffusion modeling derivatives.
This PR replaces #2211 so that maintainers can push changes into the PR during its development.
Note
At this point, the best way to communicate about this BEP is through this GitHub pull request.
[HTML preview of this BEP](XXX - will add when available)