Mne-python: eeglab events backward compatibility

[mmagnuski]

Currently when reading eeglab files I get deprecation warnings because the mne function internally uses read_events_eeglab and not the function to read events as annotations and then back as events. I am sure this is temporary, but it is annoying enough that I can volunteer to fix it. :)

And since I am volunteering - some other things regarding backwards compatibility of the new system with the new one:

• I was reading a file today that gave no events when read the old way (read then mne.find_events) but when read with the new way egglab events -> annotations -> events array gave one event (which is correct). It was not on my computer so I cannot check or share the file at the moment but I should be able in a day or so.
• I think the eeglab reader originally converted integer event types (EEG.events.type) to the same integer values in events array. Now, when turning annotations to events the events are numbered according to their presentation order and the correct values are given in a separate dict. That means that I have to correct the values myself (because otherwise every subject could have different event values) or pass event_id={str(x): x for x in range(255)} when constructing events from annotations.

[larsoner]

Marking for 0.17 since this seems like something we need to fix for release. Thanks for volunteering @mmagnuski

[agramfort]

oups... yes we need to fix this.

you may want to read
https://github.com/mne-tools/mne-python/pull/5460/files too to see
where we want to go.

[mmagnuski]

I looked through the PR you linked, I still have some questions more specific to reading eeglab files:

• do we read as digits those descriptions that are digit-only, or that contain exactly one stream of digits (so that S02 would be turned to 2 but A2S3 would be ignored ie. len(re.findall('[0-9]+', event)) == 0) @jona-sassenhagen should remember how it originally worked.
• previously, one could pass event_id_fun, not only event_id - do we still support that?
• previously events that could not be turned to digits via default event_id_fun or that were not in event_id were dropped, now they are given rather arbitrary consequent values. I think we all agree this should still work the same (throw a warning on dropping, but not assign arbitrary event values, because otherwise these would not be compatible across subjects).

[jona-sassenhagen]

do we read as digits those descriptions that are digit-only, or that contain exactly one stream of digits (so that S02 would be turned to 2 but A2S3 would be ignored ie. len(re.findall('[0-9]+', event)) == 0) @jona-sassenhagen should remember how it originally worked.

I forgot, but intuitively, making it the same as the Brainvision reader looks appropriate, doesn't it?

previously, one could pass event_id_fun, not only event_id - do we still support that?

Same: make it as BV?

previously events that could not be turned to digits via default event_id_fun or that were not in event_id were dropped, now they are given rather arbitrary consequent values. I think we all agree this should still work the same (throw a warning on dropping, but not assign arbitrary event values, because otherwise these would not be compatible across subjects).

I strongly disprefer the idea of assigning arbitrary values ...

[agramfort]

regarding your second point above @mmagnuski (auto convertion of integer event types) it seems we changed the behavior with @massich without noticing. Can you provide a file with a test to demonstrate the regression?

[agramfort]

@mmagnuski I still see event_id_func in the eeglab reader. Where did we remove it?

regarding the arbitrary numbers I agree. Please see if you can fix it and go back to the previous behavior

thanks

[larsoner]

Regarding event_id_func I think it should not be part of the new function that converts annotations to events (just in whatever io reader function must have it for backward compatibility). I think annotations should be converted to events in this function if and only if they are in event_id.

Since the new plan is to construct complete annotations on read, then allow event creation separately (rather than synthesizing an event channel during read), I don't see an advantage or reason in the long run to have event_id_func in the API, and it's not as clean an interface because it breaks the iif-in-event_id logic.

[mmagnuski]

I forgot, but intuitively, making it the same as the Brainvision reader looks appropriate, doesn't it?

@jona-sassenhagen -the BV reader expects padded values with at most 3 digits and some special characters before the digits, so it would not work for reading eeglab files. But I'll try to compare with current and past version of read_eeglab_events.

it seems we changed the behavior with @massich without noticing.

@agramfort Not exactly, read_eeglab_events seems to work ok (but I'll take a look comparing current and previous version on some files), but it raises warning urging the user to read eeglab events as annotations and then as events. And if one does so the problems I mentioned arise. I'll test it more thoroughly tomorrow and will see how to swap read_eeglab_events to the new reading scheme without changing its functionality.

[agramfort]

read_events_eeglab is deprecated. that's why you have deprecation warnings. so this is expected. thanks for taking a look @mmagnuski

@larsoner if you want to try what you suggest with stim channel please do.

[mmagnuski]

@agramfort I know, my point was that read_raw_eeglab still uses it internally (https://github.com/mne-tools/mne-python/blob/master/mne/io/eeglab/eeglab.py#L361) and that was what I originally volounteered to change. :)

[agramfort]

that's really bad :( :(

yes please send a PR so we don't use any deprecated code.

if you have some tiny bandwidth and you work a lot with eeglab files,
can you start
drafting the PR as I did for BrainVision files (#5460)

thanks a lot

[massich]

@mmagnuski yes, right now all the readers use read_events_xx still. I think that the idea is to add read_annotaions + deprecate, and then swap the code.

[massich]

previously, one could pass event_id_fun, not only event_id - do we still support that?

@mmagnuski we do not support that anymore. (see https://github.com/mne-tools/mne-python/pull/4936#discussion_r219152056), this was overcome in #5460 by charging event_id with functionality:

https://github.com/mne-tools/mne-python/blob/fa119971035fb8cf0f92acc98c846645d5f410d2/mne/io/brainvision/brainvision.py#L157-L159

[mmagnuski]

@massich Sure, I get it, but for the eeglab reader there should be a deprecation cycle, right?

[larsoner]

@mmagnuski I agree that event_id_func should continue to work as it did before in read_raw_eeglab (albeit deprecated, along with all other stim-channel-synthesizing functionality), and in the new annotations-to-events function there should be no event_id_func, only event_id.

[mmagnuski]

@agramfort continuing from the topic on annotations for sleep data:
I am not sure how what is now done by eeglab reader could be deprecated and replaced with events_from_annotations without loosing functionality. The code I pasted in the previous post (in annotations for sleep data issue) is just part of what the user would have to do by hand to get the same functionality as before. I left out some additional checks that are performed later in the code, some of which may also be present in events_from_annotations. It is also worth noting that we have many tests specific to the current implementation of eeglab reader (for example warnings and errors raised) - are we going to remove these tests too?
Note that I may be not entirely up to date with how read annotations -> create events pipeline works (for example I just noted that read_annotations_eeglab is now deprecated), but the points I raise above are related to annotations -> events step (but I may be wrong about them too :) ).
@jona-sassenhagen While I agree that .set is not a primary data format it is frequently used (many of my collaborators still use .set files) and once we invested time and code into set files support it would not be so nice to drop some functionality that we had for many versions.

[mmagnuski]

sorry, forgot to reopen.

[agramfort]

we need to come up with a solution to fix the mess we have with all EEG
readers that are inconsistent in API and should not synth a stim channel
which slows down IO etc...

do you see a solution API wise? expose an eeglab.default_event_id function
that could be passed
to events_from_annotations? had methods to Annotations object?

I am just thinking out loud....

[massich]

As @agramfort sais, whatever magic we need it can be put in a function and
pass it to events_id (and I would pass it to read_annotations directly
instead of events_from_annotations).

If I'm not wrong, this test is already in the test suit. I don't know for
which format I explicitly wrote the test from the top of my head. But I
wrote it. (the other formats had an equivalent).

So if you have a clear usecase do not easitate to share an snippet, that we
can hummer on.

Notice that further in the time we most probably need to converge this
duplicated behavior (+ this horrible event_id name for a slicing function
handle). Maybe we should write a collaborative wishlist to clean up
directions and make sure we are all on the same page.

On Wed, Nov 14, 2018, 21:05 Alexandre Gramfort [e-mail]
wrote:

we need to come up with a solution to fix the mess we have with all EEG
readers that are inconsistent in API and should not synth a stim channel
which slows down IO etc...

do you see a solution API wise? expose an eeglab.default_event_id function
that could be passed
to events_from_annotations? had methods to Annotations object?

I am just thinking out loud....

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mne-tools/mne-python/issues/5578#issuecomment-438800007,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGt-4ysKSJ4EDXyQhV6Dxg28bcIa9lP5ks5uvHejgaJpZM4XIJ8a
.

[mmagnuski]

we need to come up with a solution to fix the mess we have with all EEG
readers that are inconsistent in API and should not synth a stim channel
which slows down IO etc...

Yes, I definitely understand the motivation and like the changes, I was only afraid that some eeglab reader common functionalities could get lost.

Exposing a default_event_id could be ok enough. It would be just a little more work to import it and pass to the relevant function - but that doesn't seem to be much of a problem. If the eeglab usecase (passing specialized event_id) is explicitly stated in the docs (and surrounded with flashing lights) then the risk of people not noticing that their events are read differently aren't that high.

If I'm not wrong, this test is already in the test suit. I don't know for
which format I explicitly wrote the test from the top of my head. But I
wrote it. (the other formats had an equivalent).

Which test do you have in mind? Related to warnings and errors when reading eeglab EEG.events as
annotations?

So if you have a clear usecase do not easitate to share an snippet, that we
can hummer on.

Yes, that would probably be better than my general frermongering ;) Actually if all the warnings and errors are already dealt with when reading eeglab events as annotations (I'm not sure about that) then specialized event_id to pass to events_from_annotations would be good.

[massich]

Exposing a default_event_id could be ok enough. It would be just a little
more work to import it and pass to the relevant function - but that doesn't
seem to be much of a problem. If the eeglab usecase (passing specialized
event_id) is explicitly stated in the docs (and surrounded with flashing
lights) then the risk of people not noticing that their events are read
differently aren't that high.

You don't event need this. We can let read_annotations accept a string
called 'eeglab_default' that calls this. And do it in read instead of
events_from_annotations

On Wed, Nov 14, 2018, 21:57 Mikolaj Magnuski [e-mail]
wrote:

we need to come up with a solution to fix the mess we have with all EEG
readers that are inconsistent in API and should not synth a stim channel
which slows down IO etc...

Yes, I definitely understand the motivation and like the changes, I was
only afraid that some eeglab reader common functionalities could get lost.

Exposing a default_event_id could be ok enough. It would be just a little
more work to import it and pass to the relevant function - but that doesn't
seem to be much of a problem. If the eeglab usecase (passing specialized
event_id) is explicitly stated in the docs (and surrounded with flashing
lights) then the risk of people not noticing that their events are read
differently aren't that high.

If I'm not wrong, this test is already in the test suit. I don't know for
which format I explicitly wrote the test from the top of my head. But I
wrote it. (the other formats had an equivalent).

Which test do you have in mind? Related to warnings and errors when
reading eeglab EEG.events as
annotations?

So if you have a clear usecase do not easitate to share an snippet, that we
can hummer on.

Yes, that would probably be better than my general frermongering ;)
Actually if all the warnings and errors are already dealt with when reading
eeglab events as annotations (I'm not sure about that) then specialized
event_id to pass to events_from_annotations would be good.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mne-tools/mne-python/issues/5578#issuecomment-438815505,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGt-45hyHSMO1tRFNE4p68GMLwRVbq-Qks5uvIOegaJpZM4XIJ8a
.

[mmagnuski]

You don't event need this. We can let read_annotations accept a string
called 'eeglab_default' that calls this. And do it in read instead of
events_from_annotations.

In general - sure, good idea. But I'm not sure about doing that during read_annotations, at least with respect to annotations descriptions. I think it is better to read annotations without changing their names (descriptions), so that S 23 stays this way and convert to int (23) only when necessary (turning annotations to events).

[massich]

Or not convert it at all. You no longer need to convert 'S 23' to 23. keep it as 'S 23'. However, if you want to be backward compatible you still can by adding the right function on reading.

But all the moving parts are already in the codebase.

[mmagnuski]

Not convert to 23 when creating events array? The events array should be all integers.

[massich]

Sorry I might not explain myself proeprly. What I meant is that by the end of the day the id could be arbitrary.

See this 3 different strategies:

raw1 = read_raw_eeglab(raw_fname)
annotations = read_annotations(raw_fname)
raw1.set_annotations(annotations)
_eeglab_default_strategy = mne.io.edf._get_edf_default_event_id
for strategy in [ _eeglab_default_strategy,
                 {'S 23': 23, 'foo': 2},
                 None
                 ]):
   events, event_id = events_from_annotations(raw1, event_id=strategy)

we could have a _eeglab_default_strategy similar at what we do with edf where we strip the integer as ids if it can or it simpli adds the annotation with another consecutive id aslong as there's no present at any other nanotation.

We could pass a dictironary which does 2 things: filterout whatever is not in the dictionary and gives them an ID. In this case 'S 23' -> 23 and 'foo' -> 2. But this could be completely different. i.e: {'S 23':42, 'foo':23} is perfectly valid.

And then None to do something else. (usually colect everything in there, or apply the default)

But none of this is clear. In the sense that everything is possible, and we did not agree on anything. EDF has this _get_edf_default_event_id but brainvision and eeglab do not. None parses everything and maybe it should not.

[mmagnuski]

What I meant is that by the end of the day the id could be arbitrary.

Yes, assuming users would not use the integer values in the events. But my guess is that many do (I do for example) and that would not be backwards compatible with how eeglab events were read before.

But none of this is clear. In the sense that everything is possible, and we did not agree on anything.

I am sure we are trying to agree on something here. :) I am now only insisting on having an option to read events as they used to be read, even if the defaults change. A specific event_id function for eeglab should be ok.

[massich]

Yes exactly :) arbitrary allows for back compatibility. What I meant is
that we already have all the elements.

And on the agreeing part, I don't have any strong opinion. I'm not much of
a Mne-python user myself (yet).

We only need some complete example to ensure that we don't miss anything.
And either provide a function that generalize to the backward compatible
behavior or a dictionary.

On Thu, Nov 15, 2018, 16:11 Mikolaj Magnuski [e-mail]
wrote:

What I meant is that by the end of the day the id could be arbitrary.

Yes, assuming users would not use the integer values in the events. But
my guess is that many do (I do for example) and that would not be backwards
compatible with how eeglab events were read before.

But none of this is clear. In the sense that everything is possible, and
we did not agree on anything.

I am sure we are trying to agree on something here. :) I am now only
insisting on having an option to read events as they used to be read, even
if the defaults change. A specific event_id function for eeglab should be
ok.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mne-tools/mne-python/issues/5578#issuecomment-439073449,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGt-49egqGCTKY6rROfdLkjBInYryvvdks5uvYQQgaJpZM4XIJ8a
.

[larsoner]

FYI it looks like this is the last blocker for 0.17 release, but it's important so let's make sure we get it right

[agramfort]

@mmagnuski are you putting a veto to the release? anything you think we should change? or are we good enough?

[mmagnuski]

No, I think we have a sketch of a plan of how to deal with eeglab file reading in the future. I don't think it must be implemented before the release, we can worry about that later, because the old way of reading events still works. I will have time to tetst file reading eeglab events with read_annotations -> events_from_annotations tomorrow (but maybe this evening).

[agramfort]

great

looking forward to hear from you tomorrow :)

[mmagnuski]

Sorry for being silent - I was playing with set files from various studies in the new annotations -> events scheme and so far things work better than I thought. I still have some things to check (and some puzzling warnings/issues to dig into) but I should be able to report next week (and maybe submit a PR).

[massich]

ping @mmagnuski

[mmagnuski]

Thanks for reminding, I will get back to this. From my recent experience the biggest problem when switching from 0.17 to 0.18 dev is that in 0.17 we removed the deprecation warning for reading eeglab files and now in 0.18 reading eeglab files using event_id raises an error.

[massich]

So we mess up with the deprecation of eeglab? does it happen with the tests? how does the test handle them? sent me a snipped and I'll fix it ASAP

[mmagnuski]

So we mess up with the deprecation of eeglab?

It seems so, but I'll confirm later today. You can test it by running this code pasted from example:

import mne

data_path = mne.datasets.testing.data_path()
fname = data_path + "/EEGLAB/test_raw.set"
montage = data_path + "/EEGLAB/test_chans.locs"

event_id = {"rt": 1, "square": 2}
eog = {"FPz", "EOG1", "EOG2"}
raw = mne.io.read_raw_eeglab(fname, eog=eog, montage=montage, event_id=event_id)

If I am correct this would give no warning on 0.17 and raise an error on 0.18dev - so we've ommited a deprecation cycle for event_id. We'd have to add the deprecation to 0.18...

[larsoner]

Yes we probably only emit a warning if stim_channel is not False, but having stim_channel=False and supplying event_id should be a corner case, right? event_id will do nothing if stim_channel=False

[mmagnuski]

I've updated the code to what I think would not raise relevant deprecation warning on 0.17 (but I can't test it now).

[massich]

UPS yes, we should fix this and back-port it.

[massich]

In [1]: import mne 
   ...:  
   ...: data_path = mne.datasets.testing.data_path() 
   ...: fname = data_path + "/EEGLAB/test_raw.set" 
   ...: montage = data_path + "/EEGLAB/test_chans.locs" 
   ...:  
   ...: event_id = {"rt": 1, "square": 2} 
   ...: eog = {"FPz", "EOG1", "EOG2"} 
   ...: raw = mne.io.read_raw_eeglab(fname, eog=eog, montage=montage, event_id=event_id)                
<ipython-input-1-2d66b60eb147>:9: DeprecationWarning: stim_channel (default True in 0.17) will change to False in 0.18 and be removed in 0.19, set it to False in 0.17 to avoid this warning
  raw = mne.io.read_raw_eeglab(fname, eog=eog, montage=montage, event_id=event_id)

In [2]: import mne 
   ...:  
   ...: data_path = mne.datasets.testing.data_path() 
   ...: fname = data_path + "/EEGLAB/test_raw.set" 
   ...: montage = data_path + "/EEGLAB/test_chans.locs" 
   ...:  
   ...: event_id = {"rt": 1, "square": 2} 
   ...: eog = {"FPz", "EOG1", "EOG2"} 
   ...: raw = mne.io.read_raw_eeglab(fname, eog=eog, montage=montage, event_id=event_id, stim_channel=Fa
   ...: lse)                                                                                            

In [3]:           

we should raise both.

[mmagnuski]

And even if event_id does not do much when sim_channel is False it is more important to inform the user that event_id is going to be removed and events should read in a different way.

[mmagnuski]

@massich - this was done, right, the issue can be closed?

[agramfort]

Yes