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

BEP 011: Structural derivatives #518

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

BEP 011: Structural derivatives #518

wants to merge 2 commits into from

Conversation

effigies
Copy link
Collaborator

@effigies effigies commented Jun 30, 2020

As a follow-up to #265 (Common Derivatives), this PR adds structural derivatives. This was taken directly from the draft PR #207.

I have not made any modifications to bring it into line with the final common derivatives yet, but I wanted to get this started.

I will go through and comment up issues I see so we can discuss a bit before I push through with them.

Link to the HTML rendered BEP: https://bids-specification--518.org.readthedocs.build/en/518/05-derivatives/04-structural-derivatives.html

cc @edickie @vsiless

@effigies effigies requested a review from edickie as a code owner June 30, 2020 19:44
Comment on lines +3 to +6
## Reconstructed cortical surfaces

Reconstructed cortical surfaces should be stored as GIFTI files, and each
hemisphere should be stored separately.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we might want to generalize a little bit, as there isn't much leap from cortical surfaces to boundary-element-method (BEM) surfaces (see mri_make_bem_surfaces) like skull and skin that are not hemisphere-specific. The following text doesn't add those, but might pave the way.

Suggested change
## Reconstructed cortical surfaces
Reconstructed cortical surfaces should be stored as GIFTI files, and each
hemisphere should be stored separately.
## Reconstructed surfaces
A surface is defined by three-dimensional vertex coordinates, and a set of triangles,
or triplets of indices into the coordinate array.
Surfaces MUST be stored as [GIFTI] surface files, with one surface per file.
For structures with hemispheres, the `hemi-` entity SHOULD be used to distinguish the
surface files corresponding to each hemisphere.
For example, reconstructions of the cortical sheet are typically split into two disjoing
surfaces.

<pipeline_name>/
sub-<participant_label>/
anat/
<source_keywords>_hemi-{L|R}[_space-<surfspace>][_volspace-<volspace>][_desc-<label>]_<surftype>.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.

Do we need space and volspace? The surface file itself defines a surface space, so I suspect the interesting space here is "what volume is it embedded in". Can we think of use cases for either of space or volspace?

I think we should also add the den-<label> entity. As a (probably wrong) proposal, this might obviate the need for surface space.

And looking ahead to BEM surfaces, the hemi- might be considered RECOMMENDED not REQUIRED.

Suggested change
<source_keywords>_hemi-{L|R}[_space-<surfspace>][_volspace-<volspace>][_desc-<label>]_<surftype>.surf.gii
<source_entities>[_hemi-{L|R}][_den-<label>][_desc-<label>]_<suffix>.surf.gii

Another approach could be to separate cortical surfaces from BEM surfaces, and keep hemi REQUIRED here.

Comment on lines +29 to +38
| `<surftype>` | Description |
| ------------ | -------------------------------------------------------------------- |
| wm | The gray matter / white matter border for the cortex |
| smoothwm | The smoothed gray matter / white matter border |
| pial | The gray matter / pial matter border |
| midthickness | The midpoints between wm and pial surfaces |
| inflated | An inflation of the midthickness surface (useful for visualization) |
| vinflated | A very-inflated midthicknesss surface (also for visualization) |
| sphere | The sphere (used for registration - see transforms for nomenclature) |
| flat | The flattened surface (used for visualization) |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to consider whether to add BEM here or in a separate section.

| `dist` | Distance from a point |
| `defects` | Marked regions with surface defects |
| `sulc` | Sulcal depth |
| `myelinmap` | Myelin map calculated from T1w to T2w ratio |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we've discussed changing this to:

Suggested change
| `myelinmap` | Myelin map calculated from T1w to T2w ratio |
| `T1wT2wratio`| T1w to T2w ratio, a proxy for myelination |

Copy link
Member

Choose a reason for hiding this comment

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

+1 on this, since map should be for quantitative outputs.

@sappelhoff sappelhoff added the BEP label Jul 1, 2020
@sappelhoff sappelhoff changed the title [WIP] BEP 011: Structural derivatives BEP 011: Structural derivatives May 22, 2021
@sappelhoff sappelhoff marked this pull request as draft May 22, 2021 16:40
@rwblair rwblair mentioned this pull request Apr 21, 2022
| flat | The flattened surface (used for visualization) |

`space` filename keyword is restricted to
[Surface Coordinate Spaces](../99-appendices/08-coordinate-systems.md#Surface)
Copy link
Member

@rwblair rwblair Apr 21, 2022

Choose a reason for hiding this comment

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

Currently there is no header for #Surface or #Volume to land on at ../99-appendices/08-coordinate-systems.md

@Remi-Gau
Copy link
Collaborator

maintenance note: added a link to the rendered BEP in the top message of this PR.

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

Successfully merging this pull request may close these issues.

5 participants