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] Derived (processed) MR data #109

Merged
merged 112 commits into from
Apr 14, 2019

Conversation

chrisgorgo
Copy link
Contributor

@chrisgorgo chrisgorgo commented Dec 16, 2018

This PR culminates over two years of discussions and work on BIDS Derivatives. I derived it from our RC1 Google Document with the following changes:

  • I removed the Space JSON metadata field - it was never properly described and seemed redundant with ReferenceMap
  • I opted not to include the transformation files specification in this PR. Without specifying what file format to use to store transformations it has limited use - hopefully, it could be added later in another PR.
  • I replaced desc-manual restricted key/value pair with a JSON field (for segmentation)
  • I limited the file formats for surface data to GIFTI or CIFTI (previously they were only recommended which lead to too much flexibility).
  • I stipulated for label to use Abbreviation in the context of segmentation metadata (instead of index, context or name which was too flexible)
  • I opted not to include qform/sform in tractography metadata in this PR since it seems not well described and quite controversial.
  • Similarly, I could not find a proper description for Param1, Param2 etc. so I also left them out.
  • Limited allowed suffixes for the scalar derivatives.

Calling @effigies @satra @sgiavasis @oesteban @edickie @francopestilli @nicholst @tyarkoni @ahoopes for validation and comments. Please send suggestions via PRs to https://github.com/chrisfilo/bids-specification/tree/enh/derivatives.

I'll try to work on examples and the validator over the xmas break. Many thanks to everyone who contributed to this work.

Copy link
Collaborator

@arokem arokem left a comment

Choose a reason for hiding this comment

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

A few small comments on the dMRI part.

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
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
francopestilli
francopestilli previously approved these changes Jan 3, 2019
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
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
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
src/05-derivatives/05-diffusion-derivatives.md Outdated Show resolved Hide resolved
@TheChymera

This comment has been minimized.

| 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) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO This should be fsLR. Space has no notion of surfaces or volumes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think fsLR for this volume space defined by a volume template would be more intuitive we can change it. Pinging @edickie for second opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dunno, I don't really know MNI152NLin6AsymConte69 (I actually just register to MNI152NLin6Sym in my workflows anyway..)

| Coordinate System | Description |
| --------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| --------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| FS305 | FreeSurfer variant of the MNI305 space |
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be implicit to fsaverage* for the same reasons of MNI152NLin6AsymConte69 should not be a space. This example clearly shows how sampling should be detached from the notion of space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a label, but it should not include any special character (such as *), so if anything fsaverage can be used. I not familiar with this space though - @edickie did you add this one?

[ENH] replacing `CoordinateSystem` replacement with `space` + examples
Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Inconsistencies with L221.

pipeline/
sub-001/
anat/
sub-001_hemi-L_dparc.label.gii
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sub-001_hemi-L_dparc.label.gii
sub-001_hemi-L_dseg.label.gii

sub-001/
anat/
sub-001_hemi-L_dparc.label.gii
sub-001_hemi-R_dparc.label.gii
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sub-001_hemi-R_dparc.label.gii
sub-001_hemi-R_dseg.label.gii

pipeline/
sub-001/
anat/
sub-001_dparc.dlabel.nii
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sub-001_dparc.dlabel.nii
sub-001_dseg.dlabel.nii

sub-001/
anat/
sub-001_dparc.dlabel.nii
sub-001_dparc.dlabel.nii
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sub-001_dparc.dlabel.nii
sub-001_dseg.dlabel.nii

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah. I think there is a history of referring to atlases in nifti as segmentation and atlases on the surface as parcellations, so _dseg and _dparc suffixes appeared. but I agree that there is no reason for this.

@oesteban
Copy link
Collaborator

Hi @bids-standard/bep_leads, would it make sense to merge this PR into a branch under the upstream repo so @sappelhoff has better handles over the conversation (in a new PR)? WDYT @chrisgorgo, wouldn't that release you from moderating this PR?

@chrisgorgo
Copy link
Contributor Author

chrisgorgo commented Apr 12, 2019 via email

@sappelhoff
Copy link
Member

I have just pushed a fresh "derivatives" branch to our bids-standard/bids-specification repo. We could merge this present branch into derivatives (i.e., change the "base" of this PR and then merge it)

image

I'd prefer if @chrisgorgo could do that however, because there is this scary warning:

image

Then once we have changed base and merged this PR, we can make a new PR from the derivatives branch into master and I can keep that PR up to date.

@effigies effigies changed the base branch from master to derivatives April 12, 2019 14:21
@effigies
Copy link
Collaborator

Updated the base branch. @chrisfilo, if you can resolve conflicts, we can get this off your plate.

@effigies
Copy link
Collaborator

@sappelhoff I didn't see your request that @chrisgorgo do the base change. Very sorry about that.

@chrisgorgo chrisgorgo merged commit dd7f997 into bids-standard:derivatives Apr 14, 2019
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.