mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

Use Annotations for read_raw_egi events

Open scott-huberty opened this issue 2 years ago • 2 comments

Closes #12262

adds a parameter events_as_annotations (default False for backward compat), which will create annotations from EGI events.

@larsoner what do you think of this design and the new parameter name?

scott-huberty avatar Dec 16 '23 20:12 scott-huberty

... bonus points if this somehow takes care of https://github.com/mne-tools/mne-python/pull/11674 / https://github.com/mne-tools/mne-python/issues/11626 / https://github.com/mne-tools/mne-python/pull/11627

larsoner avatar Dec 19 '23 16:12 larsoner

... bonus points if this somehow takes care of https://github.com/mne-tools/mne-python/pull/11674 / https://github.com/mne-tools/mne-python/issues/11626 / https://github.com/mne-tools/mne-python/pull/11627

As suggested here I can quickly check if simply allowing the initial one-shot event fixes the issue without breaking a bunch of tests.

If it ends up being more complicated than that, maybe it will be a better use of time instead to get a PR up that uses mffpy which should solve those issues, as suggested here

I can report back.

scott-huberty avatar Dec 19 '23 16:12 scott-huberty

@scott-huberty we're planning to release in 3ish weeks and it would be great to get this in, let me know if you want help

larsoner avatar Jul 15 '24 18:07 larsoner

@scott-huberty we're planning to release in 3ish weeks and it would be great to get this in, let me know if you want help

I've just moved to a new city and started a postdoc last week, so I'm a bit pressed for time right now 😅.

Initially, due to time constraints, I was a little lazy with this PR since it only creates annotations using the generated stim channel.

However, it was suggested that if the new parameter events_as_annotations=True, we should not generate a stim channel. Instead, we should create the annotations directly from the event information in the file.

This has been a blocker for me because I haven't had the time to really dig into the read_raw_egi code and figure out where this event information is generated from the file.

scott-huberty avatar Jul 15 '24 19:07 scott-huberty

Okay this one should be ready for review/merge @drammock !

larsoner avatar Jul 16 '24 17:07 larsoner

Thank you @larsoner ! 🙏

scott-huberty avatar Jul 16 '24 18:07 scott-huberty

related CI failures:

_____________________________ test_egi_dig_montage _____________________________
mne/channels/tests/test_montage.py:1077: in test_egi_dig_montage
    raw_egi = read_raw_egi(egi_raw_fname, channel_naming="EEG %03d")
<decorator-gen-500>:12: in read_raw_egi
    ???
mne/io/egi/egi.py:174: in read_raw_egi
    warn(
mne/utils/_logging.py:415: in warn
    warnings.warn_explicit(
E   FutureWarning: events_as_annotations defaults to False in 1.8 but will change to True in 1.9, set it explicitly to avoid this warning

drammock avatar Jul 16 '24 19:07 drammock

Thanks for the quick review, pushed a commit to hopefully fix things @drammock !

larsoner avatar Jul 16 '24 20:07 larsoner