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] add hemi entity to schema #917

Merged
merged 6 commits into from
Nov 9, 2021
Merged

Conversation

Remi-Gau
Copy link
Collaborator

fixes #916

it had slipped through the cracks

@@ -19,6 +19,7 @@
- part
- recording
- processing
- hemi
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From my reading of the specs this is the correct place for this one.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

We don't currently include any derivative patterns in the datatype rules, so this won't show up in the entity tables or any filename templates, which definitely makes things easier.

It looks like fMRIPrep places hemi after space (see the Surfaces, segmentations and parcellations from FreeSurfer section of https://fmriprep.org/en/stable/outputs.html#functional-derivatives). I don't think really matters for this PR, since the spec already seems to say that hemi should come first, but we may want to open a follow-up issue in fMRIPrep.

The only requests I'd like to make are the updated description, as well as a link to the entity appendix section in the spec text where hemi is mentioned.

src/schema/objects/entities.yaml Outdated Show resolved Hide resolved
Remi-Gau and others added 2 commits October 30, 2021 08:28
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@Remi-Gau
Copy link
Collaborator Author

a link to the entity appendix section in the spec text where hemi is mentioned.

Done.

But I realized that the other derivatives entities have no such links in the derivatives pages. Should we add them in another PR ?

want to open a follow-up issue in fMRIPrep

Should I do it?

@@ -390,7 +390,7 @@ General fields:
"For example, **T1**: `'sub-<label>/ses-<label>/anat/"
"sub-01_T1w.nii.gz'` "
"**Surface**: `'/derivatives/surfaces/sub-<label>/ses-<label>/anat/"
"sub-01_desc-T1w_hemi-R_pial.surf.gii'` "
"sub-01_hemi-R_desc-T1w_pial.surf.gii'` "
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

small change in this example to make it bids derivatives compliant

@tsalo
Copy link
Member

tsalo commented Nov 1, 2021

But I realized that the other derivatives entities have no such links in the derivatives pages. Should we add them in another PR ?

I think that was just an oversight on my part. We should definitely link to the associated entity definition any time we reference an entity in the text.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

LGTM! I'll open an issue about the fMRIPrep derivatives.

EDIT: I've opened nipreps/fmriprep#2620.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add hemi entity to the schema
4 participants