Conversation
sappelhoff
left a comment
There was a problem hiding this comment.
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.
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
/derivativesas a dump
point 2. is probably fair enough and we'll simply have to work on BEP021.
There was a problem hiding this comment.
- are there any bids-validator functions in place yet?
Paging @rwblair.
There was a problem hiding this comment.
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.
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.
I'd need some more context as to what the question is.
There was a problem hiding this comment.
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.
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.
sappelhoff
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
| | 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.
| | 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.
| <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.
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.
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.
- 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.
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.
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.