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] Clarify use of acq and task parameters in EEG, MEG, and iEEG #188

Merged
merged 6 commits into from
Apr 26, 2019

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Mar 22, 2019

While @robertoostenveld and I went over #173, we noticed several minor inaccuracies with the entities that recording modalities can have.

This PR mainly targets a consistent and sensible use of the acq parameter, as also discussed in #194 (comment)

Furthermore, one superfluous task label is deleted from EEG electrodes.

In Robert's words:

My confusion is only with task for headshape/photo/electrodes. Those are measurements which are task-independent. See https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/03-electroencephalography.html#electrodes-description-_electrodestsv which includes task as required. I think that is incorrect. Electrode data is not recorded while the subject performs a task (except for "sit still!")

@sappelhoff sappelhoff changed the title [FIX] correct entities in several modalities [FIX] Clarify use of acq parameter in EEG, MEG, and iEEG Apr 6, 2019
@sappelhoff sappelhoff marked this pull request as ready for review April 6, 2019 07:50
@sappelhoff sappelhoff changed the title [FIX] Clarify use of acq parameter in EEG, MEG, and iEEG [FIX] Clarify use of acq and task parameters in EEG, MEG, and iEEG Apr 6, 2019
Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

A minor typo in there. I'm a little bit confused about this change, but I'll defer to your judgment. Is the problem that a particular shape of electrodes cannot be task-dependent? (as opposed to session-dependent?)

Also can you confirm that this is only updating the text to adhere to the current BIDS spec, and not changing the spec itself?

Co-Authored-By: sappelhoff <stefan.appelhoff@mailbox.org>
@sappelhoff
Copy link
Member Author

Is the problem that a particular shape of electrodes cannot be task-dependent? (as opposed to session-dependent?

Fair point - I was assuming that electrodes are set up on the participant only once per session, because I never heard of another design. But of course that doesn't mean it does not exist ... what do you say @robertoostenveld ?

Also can you confirm that this is only updating the text to adhere to the current BIDS spec, and not changing the spec itself?

Given that iEEG electrodes don't have a task label either (see here), I have the impression that in EEG this task label was erroneously copy pasted.

@choldgraf
Copy link
Collaborator

Just to be clear, those points weren't pushing back on either of those assumptions (they seem reasonable to me), just wanted to clarify them!

Copy link
Collaborator

@robertoostenveld robertoostenveld left a comment

Choose a reason for hiding this comment

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

My review aims at the general logic, not at the consistency between this table and the documentation elsewhere.

  • if acq is optional for MEG/EEG/iEEG, it should also be optional for channels.tsv. Otherwise you could have two recordings that only differ in the acq label for which you cannot distinguish the channel details. This might e.g. be the case with clinical iEEG, which sometimes gets "spliced" into different amplifiers.

  • task should not be required for eeg/ieeg electrodes (it is just like headshape and photo)

Other than that it all looks good.

@robertoostenveld
Copy link
Collaborator

Is the problem that a particular shape of electrodes cannot be task-dependent? (as opposed to session-dependent?

Fair point - I was assuming that electrodes are set up on the participant only once per session, because I never heard of another design. But of course that doesn't mean it does not exist ... what do you say @robertoostenveld ?

Agreed, I would be fine with the statement that an EEG "session" corresponds to a single application of electrodes on the subject. That does not cover all cases, but should (at this moment) cover >95%. Gel-based EEG electrodes usually require some cleanup. But an example that is not covered: using dry EEG electrodes, e.g. in a BCI experimental setup, and then reapplying the cap or using another cap.

@sappelhoff
Copy link
Member Author

if acq is optional for MEG/EEG/iEEG

The point of this PR is to remove acq for MEG/EEG/iEEG time series, because the parameter does not make sense for data that is not stable (as opposed to a picture of a face, or the T1 scan of a brain)

Thus I would argue that channels.tsv also does not need an acq parameter

task should not be required for eeg/ieeg electrodes (it is just like headshape and photo)

yep, I removed the task parameter for electrodes. see the changes

Example that is not covered: using dry EEG electrodes, e.g. in a BCI experimental setup, and then reapplying the cap or using another cap.

I think we can work with these issues when they pop up.

@sappelhoff sappelhoff merged commit 0e43a9c into bids-standard:master Apr 26, 2019
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.

4 participants