[MRG+1] Make read_eeglab_events public #4213

jona-sassenhagen · 2017-04-20T10:47:35Z

jona-sassenhagen · 2017-04-20T10:49:02Z

Is there a way to partially share docs between functions? E.g., read_raw_eeglab calls read_eeglab_events, so they have a lot of the same kwarg docs. This simple PR is 100 LOC, most of which are copied doc strings. @Eric89GXL

larsoner · 2017-04-20T12:19:11Z

No way to do it partially unfortunately

codecov-io · 2017-04-20T12:34:49Z

Codecov Report

Merging #4213 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4213      +/-   ##
==========================================
+ Coverage   86.18%   86.24%   +0.05%     
==========================================
  Files         356      357       +1     
  Lines       64337    64558     +221     
  Branches     9798     9831      +33     
==========================================
+ Hits        55451    55676     +225     
  Misses       6180     6180              
+ Partials     2706     2702       -4

jona-sassenhagen · 2017-04-20T12:39:21Z

Ready from my end.

jaeilepp

Otherwise LGTM

jaeilepp · 2017-04-20T13:07:00Z

mne/io/eeglab/eeglab.py

+                 "one event code per time point, so some events will be "
+                 "lost. You can use the function `mne.io.read_eeglab_events`"
+                 "to extract the full events array (which can then be "
+                 "passed to e.g. `mne.Epoch`."


-> mne.Epochs. And the ending parenthesis can fit to the same line.

And no reason to put backticks here, this is a warning not something Sphinx will try to render

jaeilepp · 2017-04-20T13:09:15Z

mne/io/eeglab/eeglab.py

+
+    Parameters
+    ----------
+    eeg : str | object


What does scipy.io.loadmat return? Maybe use that instead of object.

It returns a dict, but really what's expected is the ['EEG'] result of that dict, which will be an np.ndarray (even if it has ndim=0, which is what one of the conditionals is for below)

this works only for mat['eeg'] if it contains epochs, right? Or does it work also for raw ... ?

I don't know if it works for epochs. It's intended for raw.

okay I see. Can we raise an error then if it contains epochs?

I'm just worried that you will get an obscure error if you try to give it mat['eeg'] and it contains epochs. Does it contain the eeg.event field if it contains epochs?

@jasmainak I hope I didn't come across as snarky here, I just really appreciate you having written the EEGLAB importer in the first place :)

:) I think I'm not up to date with 21st century smileys yet, but no worries in any case :)

jasmainak · 2017-04-20T13:33:25Z

mne/io/eeglab/eeglab.py

-    """Create events array from EEGLAB structure.
+def read_eeglab_events(eeg, event_id=None, event_id_func='strip_to_integer',
+                       uint16_codec=None):
+    r"""Create events array from EEGLAB structure.


what does r in the beginning do?

I don't know, but it's everywhere all of a sudden! Some doc thing.

string literal, makes \ mean backslash

yes, but do we really need it? I noticed a backslash and I think that was meant to be escaped, no?

To be honest I put it there because I saw it in the other docstrings. I can remove it, but it's more consistent with the others in that file this way ...

jasmainak · 2017-04-20T13:33:48Z

mne/io/eeglab/eeglab.py

@@ -576,19 +585,74 @@ def __init__(self, input_fname, events=None, event_id=None, tmin=0,
        logger.info('Ready.')


-def _read_eeglab_events(eeg, event_id=None, event_id_func='strip_to_integer'):
-    """Create events array from EEGLAB structure.
+def read_eeglab_events(eeg, event_id=None, event_id_func='strip_to_integer',


you need to update whats_new as well

larsoner

Otherwise LGTM.

@timmidee can you see if it would work for you?

larsoner · 2017-04-20T14:07:18Z

mne/io/eeglab/eeglab.py

+                 "one event code per time point, so some events will be "
+                 "lost. You can use the function `mne.io.read_eeglab_events`"
+                 "to extract the full events array (which can then be "
+                 "passed to e.g. `mne.Epoch`."


And no reason to put backticks here, this is a warning not something Sphinx will try to render

larsoner · 2017-04-20T14:08:58Z

mne/io/eeglab/eeglab.py

+
+    Parameters
+    ----------
+    eeg : str | object


It returns a dict, but really what's expected is the ['EEG'] result of that dict, which will be an np.ndarray (even if it has ndim=0, which is what one of the conditionals is for below)

mmagnuski · 2017-04-20T15:13:56Z

looks good to me

jona-sassenhagen · 2017-04-20T18:15:29Z

whats_new conflict.

Don't you love rebasing.

jona-sassenhagen · 2017-04-20T21:20:41Z

Otherwise gtg?

jasmainak · 2017-04-20T21:37:22Z

mne/io/eeglab/eeglab.py

@@ -381,6 +381,14 @@ def __init__(self, input_fname, montage, eog=(), event_id=None,

    def _create_event_ch(self, events, n_samples=None):
        """Create the event channel."""
+        if len(set(events[:, 0])) != len(events[:, 0]):
+            warn("Some events overlap/occur at the same time sample. The "


how can this happen?

It's the problem that prompted this fix: see #3938

okay @timmidee does this solve your problem?

jasmainak · 2017-04-20T21:43:30Z

you have my +1 after @timmidee has tested it and confirmed that it works

jasmainak · 2017-04-20T21:44:08Z

Thanks a lot @jona-sassenhagen for taking a stab at this!

jona-sassenhagen · 2017-04-20T21:45:30Z

@timmidee do you know how to check out a branch from my repo ..?

jona-sassenhagen · 2017-04-20T21:45:50Z

(@jasmainak for some context, Tim and I sit and work on the same floor.)

jasmainak · 2017-04-20T21:47:37Z

(@jasmainak for some context, Tim and I sit and work on the same floor.)

ah okay! just ask him IRL then if it's too much of a hassle to pull the branch :)

jona-sassenhagen · 2017-04-20T21:48:40Z

@timmidee I won't come to the office tomorrow I think, can you perhaps share one of the original files with me?

larsoner · 2017-04-21T13:11:58Z

You also need to put this function in doc/python_reference.rst

jona-sassenhagen · 2017-04-24T17:18:28Z

Works on the problematic data by @timmidee (see http://nbviewer.jupyter.org/gist/jona-sassenhagen/eb57f5c5735a838db9fc14a8d9e536be), please merge if green.

larsoner · 2017-04-24T17:19:50Z

Comment unaddressed above, please add to python_reference.rst

jona-sassenhagen · 2017-04-24T17:21:03Z

Argh I had forgotten to save that file before git add.

agramfort · 2017-04-24T17:33:23Z

mne/io/eeglab/eeglab.py

-        events = _read_eeglab_events(eeg, event_id=event_id,
-                                     event_id_func=event_id_func)
+        events = read_events_eeglab(eeg, event_id=event_id,
+                                    event_id_func=event_id_func)


do you read all relevant info here? don't you loose str descriptions?

basically what if you don't pass event_id in param can you imagine returning an event_id to know what you read ?

I'm not sure I understand what you're saying: this one just returns integers ..?

If there are str events and no event_id, the (un-parseable) str events are dropped with a warning.

I think you could return a dictionary mapping the string descriptions to the integers, no?

I don't see how that makes sense. The problem is the following:

EEGLAB events can be arbitrary strs

MNE stim chans or mne.Epochs(..., events, ...) must be int

The user knows what events are in the data, and in case they've forgotten, we print the str events in the warning. What we have to do is, we have to allow users to map strs to ints. So in this one, the workflow goes like this:

raw = mne.io.read_raw_eeglab(fname) -> Warning: Events like the following will be dropped entirely: "square", "circle". 100 event codes could not be mapped to integers. Use the 'event_id' parameter to map such events manually. event_id = {"square":1, "circle":2} raw = mne.io.read_raw_eeglab(fname, event_id) events = mne.find_events(raw) epochs = mne.Epochs(raw, events, event_id) for cond in event_id: epochs[cond].average().plot(title=cond)

Seems fine to me. Or am I missing what you're going for?

Oh, and any event not caught by event_id_func or event_id is dropped after the warning.

It is predictable indeed, but I was just thinking that it's convenient for the user to get the event_id since the string description already exists. Otherwise, they will have to construct the dictionary manually. But it's okay if you don't have the bandwidth to implement it or you think it's unnecessary.

I don't understand ... the event_id needs to exist anyways? Otherwise the user can't pass it to the function that retrieves the events (read_raw_eeglab or read_events_eeglab).

I was thinking of an API like this:

raw = mne.io.read_raw_eeglab(fname) events, event_id = read_events_eeglab(fname) epochs = mne.Epochs(raw, events, event_id)

not sure if we're talking about the same thing :)

I don't understand how that'd work. Called without additional parameters, read_raw_eeglab and read_events_eeglab will perform a many-to-one mapping: strip every key of its non-numeric parts, and use that as the value. So it will do stuff like:

'S101' -> 101 'R101' -> 101 '101' -> 101 101 -> 101 'square' -> (dropped)

To recover square, you'd have to use event_id. To have values distinguishing between 'S101' and 'R101', you'd have to use event_id too. So in any unambiguous mapping, the user will have to construct the event_id themselves before calling the function in the first place.

timmidee · 2017-04-24T20:21:47Z

Apologies for slowing down the process. I muted the thread to focus on other work over the weekend. Thanks again for picking this up @jona-sassenhagen!

Question I have is whether what the warning says about passing the events from read_events_eeglab() to mne.Epochs() is strictly true. Will mne.Epochs() accept overlap in the events structure?

In the init for BaseEpochs the code reads:

          events = events[selected]
          if len(np.unique(events[:, 0])) != len(events):
               raise RuntimeError('Event time samples were not unique')

This will only produce an error if both of the overlapping events are specified as time-locking events ("selected"), which is certainly not unthinkable in my specific use-case.

I.e. the warning might confuse some users' next actions.

jona-sassenhagen · 2017-04-24T20:43:22Z

raise RuntimeError('Event time samples were not unique')

I didn't actually know about this.

Well, it does make sense, and also this concerns events after parsing event_id.
I guess that means read_eeglab_events should still warn if there are duplicate time points, right?

(Also note the events can also be passed to e.g. linear_regression_raw, which should support the overlap.)

timmidee · 2017-04-24T20:55:23Z

Well, it does make sense, and also this concerns events after parsing event_id.
I guess that means read_eeglab_events should still warn if there are duplicate time points, right?

Hmmmm that would seem double at first glance, could there be a case in which read_eeglab_events() would be used without using read_eeglab_events() first? Or perhaps an extra warning from read_eeglab_events() is useful if it not only states that there were duplicate time points but specifically that some functions might not accept that.

jona-sassenhagen · 2017-04-24T21:07:59Z

How about this for a warning when constructing the raw object:

Warning: {n} events will be dropped because they occur on the same time sample as another event. 
`mne.io.Raw` objects store events on an event channel, which cannot represent two events on the 
same sample. You can extract the original event structure using `mne.io.eeglab.read_events_eeglab`. 
Then, you can e.g. subset the extracted events for constructing epochs.

agramfort · 2017-04-25T07:48:20Z

ok. I had the same feeling as @jasmainak but it seems you know what you're doing :)

timmidee · 2017-04-25T07:51:06Z

How about this for a warning when constructing the raw object:

Better!

jona-sassenhagen · 2017-04-25T08:26:09Z

it seems you know what you're doing :)

I think @jasmainak 's and @agramfort 's idea is something we discussed and rejected when deciding on the API. In case you care, I'm laying out the rationale again:

for single subjects, your solution would be more convenient
for multiple subjects, there would be a mapping problem. It would not be easy to guarantee the returned event_ids are comparable, or that the same id is mapped to the same condition. A reliable way to map here would essentially be to either 1. create a hashing function taking arbitrary input and encoding it as an integer, so that it is guaranteed the same event_id works for multiple datasets; or 2. creating some magical thing that aligns multiple events and event_ids. These would be possible, but don't seem elegant.
The way we're doing it here is, we're asking the user to do a bit more work up-front - creating an event_id; but they have to create the event_id anyways for epoching. And this event_id ensures multiple datasets are mappable to each other in a way that's predictable for the user.

I'm not in principle opposed to do a function like events, event_id = read_events_eeglab(fname), but I think it's a worse solution for the much more important multi-subject case.

agramfort · 2017-04-25T08:50:12Z

fair enough !

agramfort · 2017-04-25T12:24:39Z

LGTM MRG+1

larsoner · 2017-04-25T13:38:02Z

Thanks @jona-sassenhagen

jona-sassenhagen · 2017-04-25T13:38:31Z

Thanks all.

jona-sassenhagen changed the title ~~Make read_eeglab_events public~~ WIP/FIX Make read_eeglab_events public Apr 20, 2017

jona-sassenhagen self-assigned this Apr 20, 2017

jona-sassenhagen changed the title ~~WIP/FIX Make read_eeglab_events public~~ WIP Make read_eeglab_events public Apr 20, 2017

jona-sassenhagen changed the title ~~WIP Make read_eeglab_events public~~ MRG Make read_eeglab_events public Apr 20, 2017

jaeilepp reviewed Apr 20, 2017

View reviewed changes

jasmainak reviewed Apr 20, 2017

View reviewed changes

larsoner reviewed Apr 20, 2017

View reviewed changes

jona-sassenhagen added 5 commits April 20, 2017 20:14

init make eeglab event reader public

40341cd

fix test

8324018

fix pep8

f4a64eb

remove weird non-ascii char

5f43b05

whats new/cosmit

55d88ed

jona-sassenhagen force-pushed the eeglab_events_public branch from adff520 to 55d88ed Compare April 20, 2017 18:15

jasmainak reviewed Apr 20, 2017

View reviewed changes

finalize

85b001c

jona-sassenhagen added 2 commits April 24, 2017 19:21

python_reference

88b0be6

fix import

6f492e0

agramfort reviewed Apr 24, 2017

View reviewed changes

fix test

c2ca994

timmidee mentioned this pull request Apr 24, 2017

"persistent" info per epoch? #4157

Closed

jona-sassenhagen added 2 commits April 25, 2017 11:17

change warning

c8337c3

pep8

94b7efb

agramfort changed the title ~~MRG Make read_eeglab_events public~~ [MRG+1] Make read_eeglab_events public Apr 25, 2017

larsoner merged commit 9f908e7 into mne-tools:master Apr 25, 2017

jona-sassenhagen deleted the eeglab_events_public branch April 25, 2017 13:38

[MRG+1] Make read_eeglab_events public #4213

[MRG+1] Make read_eeglab_events public #4213

Conversation

jona-sassenhagen commented Apr 20, 2017

jona-sassenhagen commented Apr 20, 2017

larsoner commented Apr 20, 2017 via email

codecov-io commented Apr 20, 2017 • edited Loading

Codecov Report

jona-sassenhagen commented Apr 20, 2017

jaeilepp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmagnuski commented Apr 20, 2017

jona-sassenhagen commented Apr 20, 2017

jona-sassenhagen commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

jasmainak commented Apr 20, 2017

jasmainak commented Apr 20, 2017

jona-sassenhagen commented Apr 20, 2017

jona-sassenhagen commented Apr 20, 2017

jasmainak commented Apr 20, 2017

jona-sassenhagen commented Apr 20, 2017

larsoner commented Apr 21, 2017

jona-sassenhagen commented Apr 24, 2017

larsoner commented Apr 24, 2017

jona-sassenhagen commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmidee commented Apr 24, 2017

jona-sassenhagen commented Apr 24, 2017

timmidee commented Apr 24, 2017

jona-sassenhagen commented Apr 24, 2017 • edited Loading

agramfort commented Apr 25, 2017 via email

timmidee commented Apr 25, 2017

jona-sassenhagen commented Apr 25, 2017

agramfort commented Apr 25, 2017 via email

agramfort commented Apr 25, 2017

larsoner commented Apr 25, 2017

jona-sassenhagen commented Apr 25, 2017

codecov-io commented Apr 20, 2017 •

edited

Loading

jasmainak Apr 20, 2017 •

edited

Loading

jona-sassenhagen commented Apr 24, 2017 •

edited

Loading