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

[FIX] Correct iEEG example that contained double suffixes #463

Merged
merged 3 commits into from
May 25, 2021

Conversation

yarikoptic
Copy link
Collaborator

As it is currently in the text, it follows no convention and introduces problematic
_double_suffix. There is also a "Talairach" subject, which suggests that may be it
was desired to have the same subject in the Talairach space? (so _space-Talairach)?

attn @sappelhoff

As it is currently in the text, it follows no convention and introduces problematic
_double_suffix.  There is also a "Talairach" subject, which suggests that may be it
was desired to have the same subject in the Talairach space? (so _space-Talairach)?
@sappelhoff
Copy link
Member

sappelhoff commented May 10, 2020

Hey @yarikoptic I am not an expert on this, but @dorahermes and @choldgraf wrote the example text (I think). Perhaps you can tag some other "space" experts?

Only comment I have: It may not be the best idea to include formatting from BEPs. In your proposed changes you refer to BEP011 --> given that a BEP is WIP work this seems a bit volatile.

PS: this is a more readable diff of GitHub's formatting of @yarikoptic changes: https://www.diffchecker.com/YRMQxEpi

@yarikoptic
Copy link
Collaborator Author

Without reference to BEP it just includes an example which follows no convention (although it is in derivatives/ so could be argued that it is ok as is, but into better to follow some convention than none)

@dorahermes
Copy link
Member

For iEEG data the electrode positions needed to be in the raw data, since these are essential for the data to be useful/interpretabl, in iEEG acquiring electrode positions is like image reconstruction for MRI. Electrode locations are most commonly obtained by integrating data from a CT, MRI, X-ray, or operative photo. Dependent on the method people use to obtain the positions, they roll out of the analysis packages in millimeter units in native space relative to a particular MRI or CT scan, in Talairach space, in pixels relative to an operative photo or in MNIxxx space. The _coordinatesystem.json clarifies this space and the intendedFor field specifies the image. At the time the iEEG spec was merged, it was decided that this image had to exist, but could live in the derivatives if it was not yet part of the main spec (e.g. Talairach/MNI). That is why the examples have the Talairach surface in the derivatives: it has to exist, but if it is not part of the main spec.

For the derivatives, we followed the suggested naming at the time. This naming scheme with Talairach has been implemented in some examples and is suggested here. We should change the suggestions when the BEP011 gets to a stable state and is close to being merged (otherwise we keep changing this example and the spec along with BEP011). I am ok with changing the suggestions if you think this part of BEP011 is stable.

Including @robertoostenveld because we had several conversations about this topic.

Can you clarify the issue with the double suffix?

@yarikoptic
Copy link
Collaborator Author

Re double suffix: eg it is in sub-01_T1w_pial.R.surf.gii it is _T1w and then second _pial. Afaik nowhere in bids and unlikely in any derivative we have two suffixes.

@dorahermes
Copy link
Member

I see, again, I think it is good to update the suggestion when the BEP011 is stable. Currently BEP011 states the following template:
<pipeline_name>/
sub-<participant_label>/
func|anat|dwi/
<source_file>_hemi-{L|R}[space-][surftype].surf.gii

what should <source_file> be
(a) sub-<>_ses-<>
(b) sub-<>_ses-<>_T1w

BEP011 currently enters (a), but this is actually not the full <source_file> and not completely unambiguous.

@robertoostenveld
Copy link
Collaborator

The use of a double suffix as in sub-01_T1w_pial.R.surf.gii in incorrect and should be fixed IMHO regardless of BEP011. It is not that BEP011 is going to introduce double suffixes.

Since the double suffix is only used in an example, I propose to update that example to something which is currently BIDS compliant, or to remove that example all-together.

Had BEP011 been merged in the spec, then we could have used an example that uses it. But right now we cannot. A dataset that claims compliance with BIDS v1.3 (current, or 1.2 that introduced iEEG AFAIK) cannot use BEP011 terminology. The spec must be internally consistent.

@nicholst nicholst changed the title ENH: tune suggested naming for the surface to folloe BEP011 ENH: tune suggested naming for the surface to follow BEP011 May 11, 2020
@dorahermes
Copy link
Member

I created a pull request on the bids-examples that used the double suffix in the _coordsystem.json files:

see #183 corrections to remove double suffix in _coordsystem.jon

@yarikoptic
Copy link
Collaborator Author

...
BEP011 currently enters (a), but this is actually not the full <source_file> and not completely unambiguous.

IMHO it should follow BEP003 in this and use <source_keywords> and then place old suffix into _desc- field:

Details from BEP003 (#265)
-   Each Derivatives filename MUST be of the form:
    `<source_keywords>[_keyword-<value>]_<suffix>.<ext>`
    (where `<value>` could either be an `<index>` or a `<label>` depending on
    the keyword)

-   When the derivatives chain involves outputs derived from a single raw input,
    `source_keywords` MUST be the entire source filename, with the omission of
    the source suffix and extension. One exception to this rule is filename
    keywords that are no longer relevant. Depending on the nature of the
    derivative file, the suffix can either be the same as the source file if
    that suffix is still appropriate, or a new appropriate value selected from
    the controlled list.

-   There is no prohibition against identical filenames in different derived
    datasets, although users should be aware of the potential ambiguity this can
    create and use the sidecar JSON files to detail the specifics of individual
    files.

-   When necessary to distinguish two files, the `_desc-<label>` keyword-value
    should be used. This includes the cases of needing to distinguish both
    differing inputs and differing outputs (e.g., `_desc-T1w` and `_desc-T2w` to
    distinguish brain mask files derived from T1w and T2w images; or `_desc-sm4`
    and `_desc-sm8` to distinguish between outputs generated with two different
    levels of smoothing).

@sappelhoff sappelhoff changed the title ENH: tune suggested naming for the surface to follow BEP011 [FIX] Correct iEEG example that contained double suffixes May 22, 2021
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

It seems like we all forgot about this PR after merging bids-standard/bids-examples#183

Yet it'd be important to get this one merged --> if only to get rid of the misleading "double suffix" use in the iEEG example.

I updated this PR with master and aligned the proposal with what was done in bids-standard/bids-examples#183

It may not be perfect, but I suggest we get this merged and continue the discussion of how it could be better in #518 and/or related BEP011 docs.

@sappelhoff sappelhoff merged commit 2b43d5b into bids-standard:master May 25, 2021
@yarikoptic yarikoptic deleted the bf-surface branch April 30, 2024 23:57
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.

5 participants