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

[MRG, DOC] Fix whats_new.rst link #1032

Merged
merged 6 commits into from
Aug 1, 2022
Merged

Conversation

mscheltienne
Copy link
Member

@mscheltienne mscheltienne commented Jul 30, 2022

Spotted in #1006

RuntimeWarning: Not setting position of 1 emg channel found in montage:
E           ['EMG']
E           Consider setting the channel types to be of EEG/sEEG/ECoG/DBS/fNIRS using inst.set_channel_types before calling inst.set_montage, or omit these channels when creating your montage.

If I read the BIDS specification correctly, a bipolar channel such as EOG, ECG, EMG should not be written to the sidecar:

Channel: [...] This can be connected to two electrodes (to measure the potential difference between them)

I amended the fix for stim channels in #1023 to include also eog, ecg and emg channels.

Also, I'm surprised the doc-build did not complain in #1023, in whats_new.rst the _ was badly placed. I fixed it here and added myself and @scott-huberty to the authors.rst file.

@welcome
Copy link

welcome bot commented Jul 30, 2022

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

@hoechenberger
Copy link
Member

Also, I'm surprised the doc-build did not complain

I had an issue with my docs in #1030 as well and the build passed. Something is off.

@mscheltienne
Copy link
Member Author

Yep, in #1023 it rendered this page without complaining about the syntax or the missing link.

image

@hoechenberger
Copy link
Member

hoechenberger commented Jul 30, 2022

If I read the BIDS specification correctly, a bipolar channel such as EOG, ECG, EMG should not be written to the sidecar:

Where did you read this?

Electrode positions should be written to electrodes.tsv. Omitting an EOG electrode during writing just because we don't handle it properly during reading is not the right solution here … if we do have EOG electrode positions, they should be written to electrodes.tsv

@mscheltienne
Copy link
Member Author

mscheltienne commented Jul 30, 2022

The distinction between Electrode or Channel:

Electrode = A single point of contact between the acquisition system and the recording site (for example, scalp, neural tissue, ...). Multiple electrodes can be organized as caps (for EEG), arrays, grids, leads, strips, probes, shafts, and so on.

Channel = A single analog-to-digital converter in the recording system that regularly samples the value of a transducer, which results in the signal being represented as a time series in the digitized data. This can be connected to two electrodes (to measure the potential difference between them), a magnetic field or magnetic gradient sensor, temperature sensor, accelerometer, and so on.

An EOG / ECG / EMG is not comprised of a single point of contact between the acquisition system and the recording site. Usually, it's a bipolar channel connected to 2 electrodes. And it does not have a location associated with it. IMO, those 2 points make it fall under the Channel category.

@hoechenberger
Copy link
Member

An EOG / ECG / EMG is not comprised of a single point of contact between the acquisition system and the recording site. Usually, it's a bipolar channel connected to 2 electrodes.

I disagree; a common scheme is to e.g. place an EOG electrode below the eye and use Fp1/Fp2 as the bipolar counterpart. If you have measured a location for those electrodes, they should end up in electrodes.tsv

I think you're confusing electrodes and channels here.

@hoechenberger
Copy link
Member

hoechenberger commented Jul 30, 2022

The specs also specifically mention the bipolar case:

Furthermore, the entries in *_electrodes.tsv and *_channels.tsv do not have to match exactly, as for example in the case of recording a single EOG channel from a bipolar referencing scheme of two electrodes, or a data channel originating from an auxiliary, non-electrode device. That is, in most cases *_electrodes.tsv will have more entries than *_channels.tsv. See the examples for *_channels.tsv below, and for *_electrodes.tsv in "Electrodes description".

@hoechenberger
Copy link
Member

In fact, their example even suggests to add EOG electrodes to electrodes.tsv even if you don't have the locations:

https://bids-specification.readthedocs.io/en/latest/04-modality-specific-files/03-electroencephalography.html#example-electrodestsv

Screen Shot 2022-07-30 at 13 48 56

So I'm -1 on this change here

@mscheltienne
Copy link
Member Author

Then what about ECG and EMG? I think all 3 should be treated in the same way: they measure muscle activity via a bipolar layout.

With ECG / EMG, an electrode from the EEG layout will (probably?) never be used as a bipolar counterpart, while for EOG it is indeed classic to use either Fp1/Fp2 as one of the counterparts, or even other electrodes. e.g. the ANT Neuro recent caps have a droplead electrode for EOG which is referenced to the same electrode as all the EEG electrodes: CPz (you can re-reference that channel to Fp1/Fp2 in your processing).

But in the end, it is still a channel with a bipolar layout, which does not measure brain activity but muscle activity, and which does not have one specific position. If for instance my EOG channel is not using Fp1/Fp2, but 2 electrodes on a bipolar aux channel, we end up in the case where it should not be included in the electrodes.tsv because the channel is comprised of 2 electrodes with 2 distinct locations. While if you use Fp1/Fp2, you can cheat and include the EOG channel because the bipolar counterpart is part of the EEG layout.

My point of view as a light-BIDS user ;) Let me know and I'll adapt accordingly. I'll return to this tomorrow or on Monday.

@sappelhoff
Copy link
Member

sappelhoff commented Jul 30, 2022

@hoechenberger
Copy link
Member

this is related to:

Hah, I already had strong opinions on this entire mess a year ago:

#485 (comment)

@hoechenberger
Copy link
Member

hoechenberger commented Jul 30, 2022

@mscheltienne

If for instance my EOG channel is not using Fp1/Fp2, but 2 electrodes on a bipolar aux channel, we end up in the case where it should not be included in the electrodes.tsv because the channel is comprised of 2 electrodes with 2 distinct locations.

I don't see why it shouldn't be included?

I don't think the specs say, "must only include electrodes that measure brain activity". It's any electrodes that go into this file…

Having two distinct electrode locations for what later becomes a single channel is precisely what electrodes.tsv is made for.

Our problem is that currently, when reading, we silently assume that electrodes.tsv contains an EEG montage, which is not necessarily the case (and not warranted by the BIDS specs)

@sappelhoff
Copy link
Member

without having read everything in this thread, my opinion is:

  • a raw data file contains data from channels, these channels should all be listed in channels.tsv ... with the corresponding order in sidecar and raw file
  • a given channel may correspond conceptually to
    • a single electrode (e.g., when referenced to a common reference, we often name the electrode and channel the same)
    • two electrodes (e.g., we often name an EOG channel "HEOG" if it's referenced between an electrode to the outer
      canthus of the left eye ("HEOG-") and another electrode to the outer canthus of the right eye ("HEOG+"))
    • note: I know that conceptually, each channel always corresponds to two electrodes (this is how we measure voltages), but it's common practice to "ignore" this when there is a common reference for all channels (such as a single electrode, or the common average reference)
  • all electrodes should be listed in electrodes.tsv
    • this includes ground, ref, bipolar electrodes, etc.
    • thus, electrodes.tsv most likely has more entries than channels.tsv

--> writing channels.tsv in an automated fashion is easy.
--> writing electrodes.tsv in an automated fashion is hard, due to the way we represent data in our analysis software (at least in mne-python ... don't know about fieldtrip, eeglab, etc.)

We should do our best in mne-bids, and then write guides for the stuff we cannot easily automate.

@hoechenberger
Copy link
Member

Idea on how to move forward here:

In addition to all (i)EEG channels, Write all channels for which we have locations to electrodes.tsv.

My thinking is that if a channel has a location, then it has a unique physical location and hence it's safe to assume it's an electrode

Thoughts?

@mscheltienne
Copy link
Member Author

Thank you for the clarification. That distinction between channels.tsv and electrodes.tsv is clear now (I knew well the electrodes.tsv file, but had forgotten the existence of channels.tsv). It does look difficult to automate the writing of the electrodes.tsv file from an mne.Info alone.

I'll revert the changes in this PR and will keep the doc fix for #1023.
And for #1006, I'll add an ignore for the warning.

@mscheltienne mscheltienne changed the title [BUG] Don't write EMG, EOG, ECG channels to electrodes.tsv sidecar [MRG, DOC] Fix whats_new.rst link Jul 31, 2022
@codecov
Copy link

codecov bot commented Jul 31, 2022

Codecov Report

Merging #1032 (12bf17f) into main (c3527fa) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1032   +/-   ##
=======================================
  Coverage   95.18%   95.18%           
=======================================
  Files          25       25           
  Lines        3778     3778           
=======================================
  Hits         3596     3596           
  Misses        182      182           

Help us with your feedback. Take ten seconds to tell us how you rate us.

@hoechenberger hoechenberger merged commit 158df44 into mne-tools:main Aug 1, 2022
@welcome
Copy link

welcome bot commented Aug 1, 2022

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@hoechenberger
Copy link
Member

Thanks @mscheltienne

@mscheltienne mscheltienne deleted the emg branch August 1, 2022 09:13
@mscheltienne mscheltienne restored the emg branch August 1, 2022 09:27
@mscheltienne mscheltienne deleted the emg branch August 1, 2022 09:28
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