-
Notifications
You must be signed in to change notification settings - Fork 163
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] Derived (processed) MR data #207
Conversation
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.
part 1/n of reviewing. I stopped at src/05-derivatives/01-introduction.md
for now. Will return soon.
src/02-common-principles.md
Outdated
example is just a convention that can be useful for organizing raw, source, and | ||
derived data while maintaining BIDS compliancy of the raw data folder. | ||
In this example **only `rawdata` subfolder and all subfolders of `derivatives`** | ||
**need to be BIDS compliant** datasets. This specification does not prescribe |
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.
So with this BEP the /derivatives
will be defined for MRI data and it makes sense to then enforce that the contents of /derivatives
is formatted according to the rules. However,
- are there any bids-validator functions in place yet?
- All other modalities will lose the "convenience" of using
/derivatives
as a dump
point 2. is probably fair enough and we'll simply have to work on BEP021.
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.
- are there any bids-validator functions in place yet?
Paging @rwblair.
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.
Looks like currently any file located in derivatives is accepted:
https://github.com/bids-standard/bids-validator/blob/master/bids-validator/bids_validator/rules/associated_data_rules.json
So there is a function currently checking the directory but, we will need to expand the regexp to account for the standard.
| MNI152Lin | Also known as ICBM (version with linear coregistration) [http://www.bic.mni.mcgill.ca/ServicesAtlases/ICBM152Lin](http://www.bic.mni.mcgill.ca/ServicesAtlases/ICBM152Lin) | | ||
| MNI152NLin6\[Sym|Asym\] | Also known as ICBM 6th generation (non-linear coregistration). Used by SPM99 - SPM8 and FSL (MNI152NLin6Sym). [http://www.bic.mni.mcgill.ca/ServicesAtlases/ICBM152NLin6](http://www.bic.mni.mcgill.ca/ServicesAtlases/ICBM152NLin6) | | ||
| MNI152NLin6\[Sym|Asym\] | Also known as ICBM 6th generation (non-linear coregistration). Used by SPM99 - SPM8 and FSL (MNI152NLin6Sym). [http://www.bic.mni.mcgill.ca/ServicesAtlases/ICBM152NLin6](http://www.bic.mni.mcgill.ca/ServicesAtlases/ICBM152NLin6) | | ||
| MNI152NLin6AsymConte69 | Template based on MNI152NLin6Asym using Conte69 data. Built by Human Connectome Project. [https://github.com/Washington-University/HCPpipelines/tree/master/global/templates](https://github.com/Washington-University/HCPpipelines/tree/master/global/templates) | |
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.
Is this really a new template or it is just the FSL MNI template with the addition of the 1.6mm isotropic sampling (and other resolutions too)? Is there a publication describing the procedure used in building the template in the case it is not a variation of existing ones?
Pinging @edickie, @satra, and also @glasserm and @tbbrown for a more informed response.
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'd need some more context as to what the question is.
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, I wasn't clear. The question is whether these templates and related data (https://github.com/Washington-University/HCPpipelines/tree/master/global/templates) are derived from the MNI template packed with FSL, some other template (e.g. the 2009 wave of templates from MNI), or were entirely constructed with the specific purposes of HCP/HCP-Pipelines.
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.
The MNI templates are based on FSL's nonlinear template.
I've made several changes to the table of template identifiers: - [x] Reordered template identifiers alphabetically - [x] Removed the distinction between surface templates and volumetric templates. After all, to properly apply a surface template one should first spatially normalize to a particular volumetric template. Although it is possible to create mixed templates (e.g. using ``MNI152Lin`` to generate a mapping for volumetric coordinates and then, say, ``fsLR`` for the surface after some surface mapping process), this option should be discouraged. Anyways, this discussion does not affect how the names of BIDS entities should be specified, since one can always describe how the template coordinates are generated with the information described in this PR. - [x] Removed ``MNI152NLin6AsymConte69`` because it does not define any new coordinate system (see bids-standard#207 (comment)), - [x] Marked DEPRECATED ``fsaverage[3|4|5|6|sym]` - [x] Added ``fsaverage`` and ``fsaverageSym`` corresponding to such deprecations. - [x] Removed ``FS305`` as it is now covered by ``fsaverage``. - [x] DEPRECATED modifiers of ``UNCInfant`` - [x] Reorganized subsections in "standard" vs "nonstandard" spaces.
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.
review part 2/n until 02-common-data-types.md
now
that were created from more than one source dataset. It is consistent with | ||
BIDS principles for the `sourcedata/` subdirectory to be used to include or | ||
reference the source dataset(s) as it existed when the derivatives were | ||
generated. Likewise, any code used to generate the derivatives from the |
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.
It is consistent with
BIDS principles for thesourcedata/
subdirectory to be used to include or
reference the source dataset(s) as it existed when the derivatives were
generated.
This sentence is not very readable, please rephrase.
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.
Please see updated text.
Hi all, what is everybody's opinion on the state of this? I know we have #213 pending, but are there any other outstanding issues? We're missing Would anybody be willing to volunteer to read through the whole thing with an eye for conflicts between sections? |
higher up in the hierarchy of the derived dataset (according to | ||
[The Inheritance Principle](../02-common-principles.md#the-inheritance-principle)) | ||
unless a particular derivative includes REQUIRED metadata fields in which case a | ||
JSON file is also 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.
I am currently working on a dataset with an anatomical T1w scan, on which I have run the freesurfer recon-all
command. That creates a directory full of files:
surfer/sub-01$ tree . -L 2
.
└── recon-all
├── bem
├── label
├── mri
├── scripts
├── src
├── stats
├── surf
├── tmp
├── touch
└── trash
Would it make sense in this case to have a surfer/sub-01/recon-all.json
file in which I specify the metadata such as the SpatialReference
pointing to my T1w scan?
--> This would mean that sidecar JSONS can also be defined for derivatives directories, not only files
gradient non-linearity / subject motion / eddy currents, intensity normalization was | ||
performed, etc.). | ||
|
||
Additional reserved JSON metadata fields: |
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.
This second part belongs in the Common Derivatives section, IMHO. cc/ @effigies @sappelhoff
|
||
| **Key name** | **Description** | | ||
| ---------------- | ------------------------------------------------------------------------------------------------------------------------------------ | | ||
| Shells | OPTIONAL. Shells that were utilized to fit the model, as a list of b-values. If the key is not present, all shells were used. | |
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.
| Shells | OPTIONAL. Shells that were utilized to fit the model, as a list of b-values. If the key is not present, all shells were used. | | |
| Shells | OPTIONAL. Shells that were utilized to fit the model, as a list of b-values. | |
Assuming that all shells were used when not present makes this key mandatory in practice.
| **Key name** | **Description** | | ||
| ---------------- | ------------------------------------------------------------------------------------------------------------------------------------ | | ||
| Shells | OPTIONAL. Shells that were utilized to fit the model, as a list of b-values. If the key is not present, all shells were used. | | ||
| Gradients | OPTIONAL. Subset of gradients utilized to fit the model, as a list of three-elements lists. If not present, all gradients were used. | |
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.
| Gradients | OPTIONAL. Subset of gradients utilized to fit the model, as a list of three-elements lists. If not present, all gradients were used. | | |
| Gradients | OPTIONAL. Subset of gradients utilized to fit the model, as a list of three-elements lists. | |
<pipeline_name>/ | ||
sub-<participant_label>/ | ||
dwi/ | ||
<source_keywords>[_space-<space>]_model-<label>[_desc-<label>]_<parameter>.nii[.gz] |
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.
<source_keywords>[_space-<space>]_model-<label>[_desc-<label>]_<parameter>.nii[.gz] | |
<source_keywords>[_space-<space>]_model-<label>[_desc-<label>]_<suffix>.nii[.gz] |
I'd stick with existing language
|
||
### Scalar maps | ||
|
||
These maps are saved 3D NIfTI files (x,y,z). Some examples of such maps are as follows: |
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'm not sure why models are all represented by a unique suffix (diffmodel
) but scalars get their own suffix. Just pointing at something we might want to discuss further.
<pipeline_name>/ | ||
sub-<participant_label>/ | ||
dwi/ | ||
<source_keywords>[_space-<space>][_desc-<label>][_subset-<label>]_tractography.[trk|tck|nii[.gz]] |
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.
Can all tractography methods be classified as either deterministic or probabilistic? I say that bc for segmentations we have dseg
and probseg
suffices
- Inhomogeneity corrected and skull stripped T1w files. | ||
|
||
- Motion-corrected DWI files. | ||
|
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.
- Time-domain filtered and ICA cleaned EEG data
|
||
| **Key name** | **Description** | | ||
| ------------- | ----------------------------------------------------------------------------------------------- | | ||
| SkullStripped | REQUIRED. Boolean. Whether the volume was skull stripped (non-brain voxels set to zero) or not. | |
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.
This cannot be a required Key Name for an EEG derivatives dataset. It can be required for MR specific derivatives, but in that case it should not be in the "common data types" section (since MEG, EEG, iEEG and Behavior also fall under the common data types).
| ----------- | ----------------------------------------------------------------------- | | ||
| index | REQUIRED. The label integer index | | ||
| name | REQUIRED. The unique label name | | ||
| abbr | OPTIONAL. The unique label abbreviation | |
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 propose to spell out abbr
into abbreviation
I've made several changes to the table of template identifiers: - [x] Reordered template identifiers alphabetically - [x] Removed the distinction between surface templates and volumetric templates. After all, to properly apply a surface template one should first spatially normalize to a particular volumetric template. Although it is possible to create mixed templates (e.g. using ``MNI152Lin`` to generate a mapping for volumetric coordinates and then, say, ``fsLR`` for the surface after some surface mapping process), this option should be discouraged. Anyways, this discussion does not affect how the names of BIDS entities should be specified, since one can always describe how the template coordinates are generated with the information described in this PR. - [x] Removed ``MNI152NLin6AsymConte69`` because it does not define any new coordinate system (see bids-standard#207 (comment)), - [x] Marked DEPRECATED ``fsaverage[3|4|5|6|sym]` - [x] Added ``fsaverage`` and ``fsaverageSym`` corresponding to such deprecations. - [x] Removed ``FS305`` as it is now covered by ``fsaverage``. - [x] DEPRECATED modifiers of ``UNCInfant`` - [x] Reorganized subsections in "standard" vs "nonstandard" spaces.
This PR supersedes bids-standard#213, and should be rebased with master after bids-standard#306 is merged. W.r.t. master it only adds the table of _Nonstandard template identifiers_ W.r.t. `common-derivatives`, the changes are: - [x] Removed the distinction between surface templates and volumetric templates. After all, to properly apply a surface template one should first spatially normalize to a particular volumetric template. Although it is possible to create mixed templates (e.g. using ``MNI152Lin`` to generate a mapping for volumetric coordinates and then, say, ``fsLR`` for the surface after some surface mapping process), this option should be discouraged. Anyways, this discussion does not affect how the names of BIDS entities should be specified, since one can always describe how the template coordinates are generated with the information described in this PR. - [x] Removed ``MNI152NLin6AsymConte69`` because it does not define any new coordinate system (see bids-standard#207 (comment)), - [x] Removed ``FS305`` as it is now covered by ``fsaverage``. - [x] Reorganized subsections in "standard" vs "nonstandard" spaces.
Merges common-derivatives immediately after removing modality-specific spec, but keeping the spec, to ease synchronization
7c6af39
to
b88cfd5
Compare
This reverts commit e5b41c0.
Important discussion to re-consider for structural derivatives: #265 (comment) |
Superseded by #518 #519 and https://github.com/bids-standard/bids-bep016. |
This replaces #109, tracking the state of the derivatives BEP as we finalize.
The current state of this branch will be rendered on https://bids-specification.readthedocs.io/en/derivatives/
Depends on #205, #167.