FIX: allow raw_fif to parse brainvision events by massich · Pull Request #6417 · mne-tools/mne-python

massich · 2019-06-05T17:03:16Z

This PR fixes this (see discussion in #6326):

raw = read_raw_brainvision(...)
raw.save(fname)
raw_read = read_raw_fif(fname)
events_from_annotations(raw_read, event_id='auto') == events_from_annotations(raw, event_id='auto') # now they are equal!!

mne/io/brainvision/tests/test_brainvision.py

mne/io/brainvision/brainvision.py

larsoner · 2019-06-05T17:09:16Z

mne/annotations.py

+        candidate = getattr(raw, '_get_auto_event_id', _DefaultEventParser)
+        if candidate == _DefaultEventParser and \
+           _check_bv_annot(raw.annotations.description):
+            return _BVEventParser()


There was an additional qualifier @agramfort wanted which was that raw be an instance of RawFIF (or I think it should also include RawArray)

That's why I started by implementing this inside a class method of Raw, but then getattr does nothing because raw is a subclass and its always present

up to some point, this part was easy: just get read of the default and the base class would already give you _DefaultEventParser.

But the method itself was weird:

@classmethod def _get_auto_event_id(cls): """Return default ``event_id`` behavior unless it matches Brainvision.""" if _check_annotation_descriptions: # self is not present return _DefaultEventParser() else: return _BVEventParser() # return a free helper function of a class that inherates from RaW.

returning _BVEventParser is not a problem perse, but it breaks the conceptual structure.

you should be able to default to _BVEventParser() only if you have a RawBrainVision, RawFIF or RawArray

If that's the contract then we need to create RawFiF which we don't have. (we only have an alias for backward compativility)

class A: pass class B(A): pass a = A() b = B() print(isinstance(a, A), isinstance(b,A), issubclass(type(b),A), issubclass(type(a),A))

True True True True

mne/annotations.py

massich

I've tried to make it as clean as I could. But I think that it would be simpler just to inspect raw.annotations.description and make the guess best on that (we have to execute _check_bv_annot anyway).

mne/annotations.py

massich · 2019-06-05T17:06:22Z

mne/annotations.py


 def _check_event_id(event_id, raw):
+    from mne.io.brainvision.brainvision import _BVEventParser
+    from mne.io.brainvision.brainvision import _check_bv_annot


I'm aware of the imports (I'll push a commit to change them)

massich · 2019-06-05T17:28:05Z

The other concern I have with this implementation is that right now is strict. Only if all the descriptors belong to 'brainvision' it would be considered brainvision. If you have some extra markers, then it would be considered not brainvision. Which to me is fine. If you have been messing up the markers, saving and reloading etc.. you should be knowing what you are doing.

codecov · 2019-06-05T17:30:02Z

Codecov Report

Merging #6417 into master will increase coverage by 0.01%.
The diff coverage is 82.6%.

@@            Coverage Diff             @@
##           master    #6417      +/-   ##
==========================================
+ Coverage   89.22%   89.24%   +0.01%     
==========================================
  Files         411      411              
  Lines       74623    74643      +20     
  Branches    12328    12331       +3     
==========================================
+ Hits        66582    66614      +32     
+ Misses       5185     5159      -26     
- Partials     2856     2870      +14

massich · 2019-06-05T17:30:37Z

I would implement it like this:

def _check_event_id(event_id, raw):
    from mne.io.brainvision.brainvision import _BVEventParser
    from mne.io.brainvision.brainvision import _check_bv_annot

    if event_id is None:
        return _DefaultEventParser()
    elif event_id == 'auto':
        if _check_bv_annot(raw.annotations.description):
            return _BVEventParser()
        else:
            return _DefaultEventParser()
    elif callable(event_id) or isinstance(event_id, dict):
        return event_id
    else:
        raise ValueError('Invalid input event_id')

massich · 2019-06-05T19:30:21Z

mne/io/brainvision/tests/test_brainvision.py

@@ -501,7 +501,7 @@ def test_read_vhdr_annotations_and_events():
    raw.annotations.append([1, 2, 3], 10, ['ZZZ', s_10, 'YYY'])


The version from e64fd4f simplifies _check_event_id and allow to remove getattr and the class methods. But requires to pass event_id because we are appending extra markers.

- _, event_id = events_from_annotations(raw) + _, event_id = events_from_annotations(raw, event_id='brainvision')

If we want to keep the default, in this case, we need to keep the class method etc. Or relax
_check_bv_annot and as soon as we detect one 'brainvision' marker, parse everything as 'brainvision'

WDYT @agramfort @jona-sassenhagen? How would you like to write the call?

massich · 2019-06-05T19:37:23Z

I've made the imports relative but they can't go outside of the function.

agramfort · 2019-06-06T08:26:01Z

mne/io/brainvision/tests/test_brainvision.py

    expected_event_id.update(YYY=10001, ZZZ=10002)  # others starting at 10001
    expected_event_id[s_10] = 10
-    _, event_id = events_from_annotations(raw)
+    _, event_id = events_from_annotations(raw, event_id='brainvision')


is this change necessary? or is it just to have this option covered by a test?

just checking if there is any public API change.

see #6417 (comment)

agramfort · 2019-06-06T09:06:26Z

mne/viz/raw.py: from ..io import RawFIF, RawArray mne/viz/raw.py: if not isinstance(raw, (RawFIF, RawArray)): RawFIF is just link to Raw but this would work no?

massich · 2019-06-06T09:40:21Z

Not as it is here:

mne-python/mne/io/__init__.py

Line 50 in 3f4c82a

from .fiff import Raw as RawFIF

This is the problem of fif being the base object.

But we can always resurrect RawFIF

massich · 2019-06-06T09:44:32Z

Scratch anything I said. RawBrainVision(BaseRaw) not Raw. Raw and RawBrainVision they are indeed siblings.

massich · 2019-06-06T10:08:29Z

I think this makes it. Merge if everyone is happy.

agramfort · 2019-06-06T11:29:36Z

good enough for me !

@massich can you see why travis complains?

massich · 2019-06-06T11:42:35Z

I would say its not related. its a precision error in lcmv.

agramfort · 2019-06-06T11:43:37Z

does this happen in other PRs?

massich · 2019-06-06T11:49:07Z

I could not replicate in my machine. so I restarted Travis to see if its just random.

~/code/mne-python bv_events*
(mne35) ❯ pytest mne/beamformer                                                    
Test session starts (platform: linux, Python 3.5.5, pytest 3.8.1, pytest-sugar 0.9.2)
rootdir: /home/sik/code/mne-python, inifile: setup.cfg
plugins: sugar-0.9.2, faulthandler-1.5.0, cov-2.6.0

 mne/beamformer/tests/test_dics.py s✓✓✓✓✓✓✓✓✓✓                                                                                19% █▉        
 mne/beamformer/tests/test_external.py ✓✓✓✓                                                                                   26% ██▋       
 mne/beamformer/tests/test_lcmv.py ✓ssss✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                   95% █████████▌
 mne/beamformer/tests/test_rap_music.py ✓✓✓                                                                                  100% ██████████
------------------------------------- generated xml file: /home/sik/code/mne-python/junit-results.xml --------------------------------------


========================================================= short test summary info ==========================================================
SKIP [1] mne/beamformer/tests/test_dics.py:107: Test test_make_dics skipped, requires h5py. Got exception (No module named 'h5py')
SKIP [4] mne/beamformer/tests/test_lcmv.py:199: Test test_make_lcmv skipped, requires h5py. Got exception (No module named 'h5py')
======================================================== slowest 20 test durations =========================================================
3.00s call     mne/beamformer/tests/test_lcmv.py::test_lcmv_vector
2.58s call     mne/beamformer/tests/test_lcmv.py::test_lcmv_reg_proj[unit-noise-gain-True]
2.54s call     mne/beamformer/tests/test_lcmv.py::test_lcmv_reg_proj[unit-noise-gain-False]
2.46s call     mne/beamformer/tests/test_dics.py::test_tf_dics[testing_data]
2.21s call     mne/beamformer/tests/test_lcmv.py::test_lcmv_reg_proj[nai-True]
2.03s call     mne/beamformer/tests/test_lcmv.py::test_lcmv_reg_proj[None-False]
2.00s call     mne/beamformer/tests/test_lcmv.py::test_lcmv_reg_proj[nai-False]
1.97s call     mne/beamformer/tests/test_lcmv.py::test_tf_lcmv
1.96s call     mne/beamformer/tests/test_lcmv.py::test_lcmv_reg_proj[None-True]
1.55s call     mne/beamformer/tests/test_dics.py::test_apply_dics_timeseries[testing_data]
1.46s call     mne/beamformer/tests/test_rap_music.py::test_rap_music_simulated
1.44s call     mne/beamformer/tests/test_rap_music.py::test_rap_music_sphere
1.34s call     mne/beamformer/tests/test_lcmv.py::test_lcmv_source_power
1.25s setup    mne/beamformer/tests/test_external.py::test_lcmv_fieldtrip[testing_data-ug_vec-None-None-False]
1.08s setup    mne/beamformer/tests/test_external.py::test_lcmv_fieldtrip[testing_data-ung-unit-noise-gain-max-power-False]
1.02s call     mne/beamformer/tests/test_lcmv.py::test_depth_does_not_matter[testing_data-testing_data-max-power-nai]
0.99s call     mne/beamformer/tests/test_lcmv.py::test_depth_does_not_matter[testing_data-testing_data-max-power-unit-noise-gain]
0.97s setup    mne/beamformer/tests/test_external.py::test_lcmv_fieldtrip[testing_data-ug_scal-None-max-power-False]
0.96s call     mne/beamformer/tests/test_lcmv.py::test_lcmv_ctf_comp
0.94s call     mne/beamformer/tests/test_lcmv.py::test_depth_does_not_matter[testing_data-testing_data-vector-nai]

Results (73.02s):
      53 passed
       5 skipped

massich · 2019-06-06T12:08:41Z

It passed here https://travis-ci.org/mne-tools/mne-python/jobs/542190809#L2724

Travis should be green this time.

massich · 2019-06-06T12:36:10Z

@jona-sassenhagen does this solve your usecase?

larsoner · 2019-06-06T16:39:40Z

@jona-sassenhagen feel free to merge if you're happy

jona-sassenhagen · 2019-06-07T11:30:23Z

mne/annotations.py

-        return getattr(raw, '_get_auto_event_id', _DefaultEventParser)()
+        if isinstance(raw, RawBrainVision) or (
+            isinstance(raw, (RawFIF, RawArray)) and
+            _check_bv_annot(raw.annotations.description)


Maybe we should log/warn in case the BV parser is called on a RawFIF/RawArray?

I would not warn because the save-load round-trip should be a reasonable use case. But a logger.info seems fine

jona-sassenhagen · 2019-06-07T11:32:43Z

mne/io/brainvision/brainvision.py

+    markers_basename = set([dd.rstrip('0123456789 ') for dd in descriptions])
+    bv_markers = (set(_BV_EVENT_IO_OFFSETS.keys())
+                  .union(set(_OTHER_ACCEPTED_MARKERS.keys())))
+    return len(markers_basename - bv_markers) == 0


We could be even stricter here if I understand correctly that the numeric code ending a BV code is always 3 characters (whitespace left padded plus an int 0-255), i.e.,

set([dd[:-3] for dd in descriptions])

@cbrnr this is the case right?

I think so, at least I've only ever seen markers like that (S 2, R 1, ...).

I started by set([dd[:-3] for dd in descriptions]) but I went for rstrip because I found it more readable. But either one is fine.

jona-sassenhagen · 2019-06-07T11:34:44Z

Thanks for doing this @massich . If you notify when you've addressed my two short comments, I would merge.

massich · 2019-06-11T14:35:36Z

I've also rebased. Feel free to merge.

jona-sassenhagen · 2019-06-11T15:25:34Z

The Travis builds are failing?

larsoner · 2019-06-11T15:34:33Z

Yes and the failure is real

https://travis-ci.org/mne-tools/mne-python/jobs/544263546

massich · 2019-06-11T16:29:32Z

yes I think that sync or some other had some extra space. rstrip should work

larsoner · 2019-06-11T18:16:12Z

Thanks @massich

jona-sassenhagen · 2019-06-11T18:17:07Z

Thanks @massich !

* TST: usecase * FIX: check annotation description base names * fix: remove pathlib * RFC: Alternative without getattr. * imports * Fix: avoid adding 'barinvision' as event_id keyword * remove _get_auto_event_id * update whatsnew * Update brainvision.py * cosmit * fix * use rstrip

* FIX: allow raw_fif to parse brainvision events (#6417) * TST: usecase * FIX: check annotation description base names * fix: remove pathlib * RFC: Alternative without getattr. * imports * Fix: avoid adding 'barinvision' as event_id keyword * remove _get_auto_event_id * update whatsnew * Update brainvision.py * cosmit * fix * use rstrip * MRG+1, DOC: footer logos (#6420) * add individual inst. logos; add uni-graz * add image titles (hover) * force add missing logos * codespell skip SVG files * fix: make inria PNG; better logo wrapping * fix footer overlap; slightly larger logos * STY: prefer _DefaultEventParser to _Counter (#6355) * Fix docstrings (#6385) * docstring deduplication * more n_jobs * fix: import fill_doc * fix crossrefs; deduplicate docdict def for random_state * FIX: Conftest [circle front] * FIX: Ref [circle front]

jona-sassenhagen · 2019-06-12T13:26:49Z

Oh wow, I found a file that's become unreadable with this PR!

    826         pat = re.compile(r'^(\s*def\s)|(\s*async\s+def\s)|(.*(?<!\w)lambda(:|\s))|^(\s*@)')
    827         while lnum > 0:
--> 828             if pat.match(lines[lnum]): break
    829             lnum = lnum - 1
    830         return lines, lnum

IndexError: list index out of range

I am investigating.

larsoner reviewed Jun 5, 2019

View reviewed changes

massich commented Jun 5, 2019

View reviewed changes

agramfort reviewed Jun 6, 2019

View reviewed changes

agramfort approved these changes Jun 6, 2019

View reviewed changes

jona-sassenhagen reviewed Jun 7, 2019

View reviewed changes

Joan Massich added 7 commits June 11, 2019 16:34

TST: usecase

f59989e

FIX: check annotation description base names

00c2a5a

fix: remove pathlib

3bd7de8

RFC: Alternative without getattr.

8d8864e

imports

05cfffd

Fix: avoid adding 'barinvision' as event_id keyword

bdb03a0

remove _get_auto_event_id

ff8d328

Joan Massich and others added 4 commits June 11, 2019 16:34

update whatsnew

18a9c98

Update brainvision.py

7dca397

cosmit

f1ed53a

fix

f4de5e2

massich force-pushed the bv_events branch from edbd0c6 to f4de5e2 Compare June 11, 2019 14:35

use rstrip

ff7fac0

larsoner merged commit a420019 into mne-tools:master Jun 11, 2019

larsoner mentioned this pull request Jun 11, 2019

MAINT: 0.18.2 backports #6449

Merged

massich deleted the bv_events branch August 7, 2019 10:09

		@@ -501,7 +501,7 @@ def test_read_vhdr_annotations_and_events():
		raw.annotations.append([1, 2, 3], 10, ['ZZZ', s_10, 'YYY'])

Uh oh!

Conversation

massich commented Jun 5, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich Jun 5, 2019 • 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!

massich Jun 6, 2019 • 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!

Uh oh!

massich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich commented Jun 5, 2019

Uh oh!

codecov bot commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

massich commented Jun 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich commented Jun 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Jun 6, 2019 via email • edited by massich Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

massich commented Jun 6, 2019

Uh oh!

massich commented Jun 6, 2019

Uh oh!

massich commented Jun 6, 2019

Uh oh!

agramfort commented Jun 6, 2019

Uh oh!

massich commented Jun 6, 2019

Uh oh!

agramfort commented Jun 6, 2019 via email • edited by massich Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

massich commented Jun 6, 2019

Uh oh!

massich commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

massich commented Jun 6, 2019

Uh oh!

larsoner commented Jun 6, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich Jun 5, 2019 •

edited

Loading

massich Jun 6, 2019 •

edited

Loading

codecov bot commented Jun 5, 2019 •

edited

Loading

agramfort commented Jun 6, 2019 via email •

edited by massich

Loading

agramfort commented Jun 6, 2019 via email •

edited by massich

Loading

massich commented Jun 6, 2019 •

edited

Loading