WIP, ENH: BrainVision impedance parsing #7771

honzaseda · 2020-05-12T15:39:44Z

I was using the MNE tool for parsing BrainVision data files in a paper where we needed to parse all the information from the segmentation and impedance sections of a header file. As i did not find these informations in the current raw parsed data, an enhancement of the current parser was implemented that adds these sections to the raw info.

Considering a possible wider use case for these changes, we decided that this enhancement could be included in the tool directly.

Enhancements:

The function parse_segmentation takes any of the key-value pairs in the segmentation subsections of the BrainVision file starting with S e g m e n t a t i o n / A v e r a g i n g, as well as some of the added key-values in the Common Infos. The segmentation is stored in a new info item segmentation.

The function parse_impedance creates a dict of all the channel impedances from the header file. All of the channels in the raw info are iterated over, and if an impedance record exists, they are appended to the channel dict.

The header file that these were tested on is in mne/io/brainvision/tests/data/test_segmentation_impedance.vhdr (also used for the unit tests)

Problems:

However, even though these changes work well in our local environment and the parsing of the values in itself works good, implementing these changes directly to MNE causes some of the already existing unit tests to fail, and I'm not really sure how to solve these issues. When i try to add any new attributes to the info structure, even an empty list.

These are the failing tests:

FAILED mne/io/brainvision/tests/test_brainvision.py::test_brainvision_data_highpass_filters - RuntimeWarning: Could not get size for self.info
FAILED mne/io/brainvision/tests/test_brainvision.py::test_brainvision_data_lowpass_filters - RuntimeWarning: Could not get size for self.info
FAILED mne/io/brainvision/tests/test_brainvision.py::test_brainvision_data_partially_disabled_hw_filters - AssertionError: assert {'acq_pars', ... 'comps', ...} == {'acq_pars', ... 'comps', ...}
FAILED mne/io/brainvision/tests/test_brainvision.py::test_brainvision_data_software_filters_latin1_global_units - AssertionError: assert {'acq_pars', ... 'comps', ...} == {'acq_pars', ... 'comps', ...}
FAILED mne/io/brainvision/tests/test_brainvision.py::test_brainvision_data - RuntimeWarning: Could not get size for self.info

Any ideas where the problem could be for the existing tests and how this could be improved?

larsoner · 2020-05-12T15:43:44Z

As i did not find these informations in the current raw parsed data, an enhancement of the current parser was implemented that adds these sections to the raw info.

The set of values / things that can be added to Info is restricted by the FIF format spec, so we can't add things there arbitrarily. Instead it might be worth adding these to the RawBrainvision class as attributes or methods, such as raw_bv.get_impedances() or so. They'll be lost on save-read round-trip (to FIF) but that shouldn't be too much of a problem for getting things like impedances.

honzaseda · 2020-05-12T15:52:06Z

Thanks for the suggestion, I will look into this.

honzaseda · 2020-05-12T18:08:50Z

Moved the parsed data into attributes of the RawBrainvision class

agramfort

apparently they're two features here. The ability to read general metadata on channels and reading in averaged data ie Evoked objects in brainvision format.

agramfort · 2020-05-12T18:34:49Z

mne/io/brainvision/brainvision.py

    return len(markers_basename - bv_markers) == 0
+
+
+def parse_impedance(settings):


this should be a private function

Suggested change

def parse_impedance(settings):

def _parse_impedance(settings):

@honzaseda you forgot to make this method private as well

mne/io/brainvision/brainvision.py

agramfort · 2020-05-12T18:36:35Z

mne/io/brainvision/tests/data/test_segmentation_impedance.vhdr

+SegmentationType=MARKERBASED
+SegmentDataPoints=1100
+Averaged=YES
+AveragedSegments=80


does it mean it should be read as an Evoked dataset?

Yes, if Averaged is set to YES, it means that the data file contains data that have been averaged across segments.

this tells me that we should add a read_evoked_brainvision no?
I would rather not have raw_raw_brainvision read such a header without complaining

Probably? I'm not really sure as i do not know the inner workings of MNE that well.

agramfort · 2020-05-12T20:07:54Z

@sappelhoff or @cbrnr can you pitch in?

sappelhoff

Hi @honzaseda, thanks for contributing these enhancements. --> might have been easier to try to include one at a time though :-)

I only had a look at the impedance parsing so far and that looks okay to me as it is. I am just not sure whether it's a good idea to make the impedances an attribute of raw, because BrainVision raw files will be the only files to have this attribute?

The alternative would be to optionally return the impedances as a data table at read time. e.g. raw, impedances = mne.io.read_raw_brainvision(path, return_impedances=True)

re: segmentation (not having checked the code yet) --> I never met somebody who used this feature of the BrainVision Recorder software :-) ... first time, yay.

cbrnr · 2020-05-14T16:06:08Z

There is raw._raw_extras to store this kind of stuff. For example, almost all of the EDF header goes into that attribute. Not sure if putting it in that dict is a better option than creating a raw.impedances attribute or a getter method.

larsoner · 2020-05-14T16:14:22Z

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.

The alternative would be to optionally return the impedances as a data table at read time

This would also be okay, but it's not all that extensible of there are other attributes people will want from other readers. You end up with a bunch of return_... kwargs or otherwise changing the API signature. Seems cleaner than just

There is raw._raw_extras to store this kind of stuff.

Users should not expect private attributes to be stable, they can be changed in a backward compatible way at any time during development with no warning. So we should not advertise that people store information they want in there, or make use of private attributes in general.

BTW ideally / by design the only things that should actually live in _raw_extras are the things you need to actually _read_segment_file.

cbrnr · 2020-05-14T16:18:46Z

Yes, if this should be used by users it's better not to rely on that private attribute. I'd vote for a simple impedances attribute then (instead of a getter method).

agramfort · 2020-05-14T17:22:59Z

one strategy could be to use the chs['loc'][6] entry for impedance. We've come up with such a strategy to store wavelength for fNIRS data.

sappelhoff · 2020-05-14T19:10:35Z

one strategy could be to use the chs['loc'][6] entry for impedance.

can we have unlimited space in the chs['loc'][X] entries? Or is this somehow bounded by FIFF rules?

If it's bounded, using the limited space for impedance values would seem like a waste to me.

If it's not bounded +1

Otherwise, with Eric's description I know also vote for a simple attribute.

And I also vote for @honzaseda to split the two enhancements into 2 PRs =)

agramfort · 2020-05-14T19:16:06Z

chs pos is 12 values for all channels. for MEG is 3x3 rotation + 3 translation for EEG it's 6 : location of channel + location of reference. So technically we have 6 spare entries unused

…

honzaseda · 2020-05-15T14:43:38Z

Removed the segmentation parsing, I will create a separate PR for this and leave only the impedance parsing here.

Left the impedances as an attribute of the RawBrainvision class for now.

larsoner · 2020-05-15T15:21:13Z

mne/io/brainvision/tests/data/test_segmentation_impedance.vhdr

+
+Channels
+--------
+#     Name      Phys. Chn.    Resolution / Unit   Low Cutoff [s]   High Cutoff [Hz]   Notch [Hz]    Series Res. [kOhm] Gradient         Offset


See comment in other PR, this should eventually be added to mne-testing-data

for the sake of this PR you could add the impedances to one of the existing VHDR files in https://github.com/mne-tools/mne-python/tree/master/mne/io/brainvision/tests/data and then use that file for testing.

sappelhoff · 2020-07-01T11:19:06Z

just a note, this PR could also come in handy for MNE-BIDS to automatically populate the impedance field in electrodes.tsv BIDS files.

We're having a short "unconference" at my institute soon, and maybe I'll find time there to finish this. Unless you have other plans @honzaseda ?

sappelhoff · 2020-07-07T13:30:44Z

We're having a short "unconference" at my institute soon, and maybe I'll find time there to finish this. Unless you have other plans @honzaseda ?

I just went ahead and finished it, and included you in the credits. Thanks for making a start with this PR @honzaseda!

See #7974

Added BrainVision impedance and segmentation parsing

ba6bb08

Jan Sedivy added 2 commits May 12, 2020 18:26

Moved the parsed sections to RawBrainvision class attributes

457d7ca

Code style

d665848

honzaseda marked this pull request as ready for review May 12, 2020 18:11

agramfort reviewed May 12, 2020

View reviewed changes

Made the functions private. Following numpydoc standard

620b0ac

sappelhoff reviewed May 14, 2020

View reviewed changes

Removed segmentation parsing, added missing private function

6403054

honzaseda changed the title ~~WIP, ENH: BrainVision impedance and segmentation parsing~~ WIP, ENH: BrainVision impedance parsing May 15, 2020

honzaseda mentioned this pull request May 15, 2020

WIP, ENH: BrainVision segmentation parsing #7792

Closed

larsoner reviewed May 15, 2020

View reviewed changes

sappelhoff mentioned this pull request Jul 7, 2020

MRG, ENH: BrainVision impedance parsing #7974

Merged

larsoner closed this in #7974 Jul 9, 2020

		return len(markers_basename - bv_markers) == 0


		def parse_impedance(settings):

	def parse_impedance(settings):
	def _parse_impedance(settings):

Uh oh!

WIP, ENH: BrainVision impedance parsing #7771

WIP, ENH: BrainVision impedance parsing #7771

Uh oh!

Conversation

honzaseda commented May 12, 2020

Uh oh!

larsoner commented May 12, 2020

Uh oh!

honzaseda commented May 12, 2020

Uh oh!

honzaseda commented May 12, 2020

Uh oh!

agramfort left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented May 12, 2020

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

cbrnr commented May 14, 2020

Uh oh!

larsoner commented May 14, 2020

Uh oh!

cbrnr commented May 14, 2020

Uh oh!

agramfort commented May 14, 2020 via email

Uh oh!

sappelhoff commented May 14, 2020

Uh oh!

agramfort commented May 14, 2020 via email

Uh oh!

honzaseda commented May 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sappelhoff commented Jul 1, 2020

Uh oh!

sappelhoff commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

agramfort left a comment •

edited

Loading