Skip to content

Conversation

@sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Jul 7, 2020

takes over #7771
closes #7771

notes for posterity

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

I am wondering if we should think about adding support for this in the fif standard.

@larsoner @mkajola jnenonen any thought on adding EEG impedance values to fiff format?

@sappelhoff
Copy link
Member Author

sappelhoff commented Jul 8, 2020

The tests are failing because object_diff is comparing np.nan as False:

In [4]: np.nan == np.nan                                                        
Out[4]: False

so actually, everything is passing. How should I fix this?

EDIT: Okay, I think I found why --> object_diff did so far not deal with non-array np.nan floats. Should be fixed now.

@sappelhoff sappelhoff changed the title ENH: BrainVision impedance parsing MRG, ENH: BrainVision impedance parsing Jul 8, 2020
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

the self.impedances attribute should be added to the attributes documentation of the Raw class.

see https://mne.tools/dev/generated/mne.io.Raw.html#mne.io.Raw

if should be None if not present. I would like to have a special case of a Raw where code work with a RawBranvision instance but breaks with RawEDF etc.

the common test for raw object should also be amended.

how do you see the pb of now saving the attributes on the file with raw.save?

@sappelhoff
Copy link
Member Author

these points go back to the discussion we had some weeks ago.

This is what @larsoner said about this in his comment

BrainVision raw files will be the only files to have this attribute?

I think this is okay. It will also be lost on FIF round-trip but that's also okay. If there are other readers that support impedances we can think about a more global way of storing them or something. As long as we pick a sensible attribute here we can always extend it to other readers later via a mixin or something.


If raw.impedances becomes a publicly documented attribute of Raw, then we also HAVE to implement proper saving of this attribute, right?

So the options I see:

  1. make it an undocumented attribute of BrainVision files only, until we get more file format readers to parse impedances
  2. make it a fully documented attribute of Raw, with possible FIF roundtrip

With option 2., I am a bit uncomfortable, because I don't know how many file formats even support saving the impedance values in their file formats?

I have only ever seen this for BrainVision files. Do you know of more @hoechenberger @cbrnr ?

@agramfort
Copy link
Member

agramfort commented Jul 8, 2020 via email

@sappelhoff
Copy link
Member Author

does eeglab set files have a documented way of storing this info?

I did a quick google search, and not alot shows up. But I am no expert on .set

EDF files also do NOT seem to have impedances: https://www.edfplus.info/specs/edfplus.html

@hoechenberger
Copy link
Member

I have only ever seen this for BrainVision files. Do you know of more @hoechenberger @cbrnr ?

I've only worked with BrainVision and BioSemi data, and I only remember seeing this for BV!

@cbrnr
Copy link
Contributor

cbrnr commented Jul 8, 2020

I haven't seen that either. If you want to make it a documented attribute, then the default could be None (meaning for all formats except BrainVision). Even BrainVision files normally don't have this kind of information, so it should really be an optional piece of information.

@hoechenberger
Copy link
Member

hoechenberger commented Jul 8, 2020

Even BrainVision files normally don't have this kind of information

Oh really? Interesting, and good to know! The ones produced in our lab always contained this info. Do you know under which circumstances this info will / will not be produced?

@cbrnr
Copy link
Contributor

cbrnr commented Jul 8, 2020

Maybe I'm wrong, but this should only work for amps that measure impedances (i.e. active electrodes) - so you won't find this info for passive amps. Also, none of our example files in the testing data repo has impedance info.

@hoechenberger
Copy link
Member

Maybe I'm wrong, but this should only work for amps that measure impedances (i.e. active electrodes)

Ah that could be right, I've only used active electrodes in the past years!

@sappelhoff
Copy link
Member Author

whether or not impedances are saved with the BrainVision files really depends on a lot of things.

  • the amplifier/electrodes being used
  • the recording software and settings being used (BrainVision Recorder works, but also not in all settings/impedance-checks)
  • whether or not you have a USB connection to the "actiCap control box"(interface between amps and electrodes) vs only the fibre optics cables

For me active vs passive was not a deciding factor --> I can record impedances from both activa AND passive electrodes (see e.g., the HEOG+ and HEOG- impedances I added in this PR --> that's real data from passive electrodes)


anyhow, it seems like reading impedance values from EEG file formats may prove to be difficult.

possible not possible
BrainVision (sometimes) EDF
BDF
.set

What about FIF?

@larsoner
Copy link
Member

larsoner commented Jul 8, 2020

Not that I know of

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

can you at least update https://github.com/mne-tools/mne-python/blob/master/mne/io/brainvision/brainvision.py#L52 to document attributes?

then let's reconsider making impedances first class citizens in mne when more formats support this...

Attributes
----------
impedances : dict
A dictionary of all electrodes and their impedances.
Copy link
Member

Choose a reason for hiding this comment

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

FYI we don't document the individual classes so this will only show up if you look at the doc of this class __init__ in a terminal or IDE (which is probably slim to never). But I guess we can keep it

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@larsoner larsoner merged commit 05df66f into mne-tools:master Jul 9, 2020
@larsoner
Copy link
Member

larsoner commented Jul 9, 2020

Thanks @sappelhoff

@sappelhoff sappelhoff deleted the imp_parsing branch July 9, 2020 20:18
@sappelhoff
Copy link
Member Author

seems like the GDF format supports impedance values: https://arxiv.org/pdf/cs/0608052v6,

but I have never used that format and have not seen it "in the wild" too much.

larsoner added a commit to larsoner/mne-python that referenced this pull request Jul 14, 2020
* upstream/master: (21 commits)
  MRG: Add SSP projectors to Report (mne-tools#7991)
  BUG: Fix warning (mne-tools#8006)
  WIP: TFR Doc/test changes (mne-tools#7998)
  MAINT: Remove numpydoc test on 3.6 (mne-tools#8005)
  MAINT: Better error message for mismatch (mne-tools#8007)
  MRG: Allow removal of active projectors if channels they applied to have meanwhile been dropped (mne-tools#8003)
  MRG Freesurfer coordinate frame tutorial (mne-tools#7578)
  FIX: Fix stockwell checks (mne-tools#7996)
  MRG, ENH: Add array-spacing plugin and reorganize deps (mne-tools#7997)
  MRG, ENH: Reduce memory usage of Welch PSD (mne-tools#7994)
  STY: One more [ci skip]
  STY: Docstyle [ci skip]
  Report parsing (mne-tools#7990)
  MRG, ENH: BrainVision impedance parsing (mne-tools#7974)
  BUG: Fix missing source space points (mne-tools#7988)
  [MRG] Strip base directory name from Report captions when using parse_folder (mne-tools#7986)
  DOC: Update estimates [skip travis] (mne-tools#7987)
  DOC: Try to improve find_bad_channels_maxwell doc (mne-tools#7982)
  VIZ, BUG: fix tickmarks in evoked topomap colorbar (mne-tools#7980)
  Add low-pass filter to find_bad_channels_maxwell() (mne-tools#7983)
  ...
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