Use Annotations for read_raw_egi events by scott-huberty · Pull Request #12300 · mne-tools/mne-python

scott-huberty · 2023-12-16T20:55:13Z

Closes #12262
Closes #11674
Closes #11672

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?

mne/io/egi/egi.py

larsoner · 2023-12-19T16:17:26Z

mne/io/egi/egimff.py

+            annot["onset"].extend(ev[:, 0] / self.info["sfreq"])
+            annot["duration"].extend(np.zeros(ev.shape[0]))
+            annot["description"].extend([event_dict[e] for e in ev[:, 2]])
+            self.set_annotations(Annotations(**annot))


I would go a step further and when events_as_annotations=True is used, do not create the synthetic event channel and raw.event_id

Sure, since those synthetic channels are already created by the time it reaches my code, I can delete them if events_as_annotations is true

Rather than create then delete, would be better to avoid creation in the first place

Hmm ok. Then i'll need to take a new approach to this PR! Since now I am just using mne.find_eventsand convert those events to annotations.

mne/io/egi/tests/test_egi.py

larsoner

Looks like a good start!

larsoner · 2023-12-19T16:20:21Z

... bonus points if this somehow takes care of #11674 / #11626 / #11627

scott-huberty · 2023-12-19T16:48:20Z

... bonus points if this somehow takes care of #11674 / #11626 / #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.

larsoner · 2024-07-15T18:56:16Z

@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

scott-huberty · 2024-07-15T19:32:50Z

@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.

* upstream/main: (252 commits) Disable the "Back to top" button in the documentation (mne-tools#12688) DOC: match_channel_orders works on Epochs and Evoked, too (mne-tools#12699) Scale points and labels in montage plot (mne-tools#12703) Add license header to mne.stats.erp (mne-tools#12712) Update license year to 2024 (mne-tools#12713) Add standardized measurement error (SME) (mne-tools#12707) ENH: Parallel example execution in doc build (mne-tools#12708) MAINT: Update PR template (mne-tools#12692) MAINT: Fix doc build (mne-tools#12706) [pre-commit.ci] pre-commit autoupdate (mne-tools#12702) Improve documentation of ylim argument through Evoked plotting function (mne-tools#12697) [pre-commit.ci] pre-commit autoupdate (mne-tools#12696) BUG: Fix bug with CSP rank="full" (mne-tools#12694) MRG: Add epochs metadata summary to HTML representation (mne-tools#12686) Correct `Epochs.apply_function` docstring (mne-tools#12691) FIX: Gracefully handle missing datetime in Eyelink File (mne-tools#12687) MAINT: Restore SciPy pre (mne-tools#12689) Enh single channel annotation (mne-tools#12669) [pre-commit.ci] pre-commit autoupdate (mne-tools#12682) Bump autofix-ci/action from 1.2 to 1.3 in the actions group (mne-tools#12681) ...

larsoner · 2024-07-16T17:17:15Z

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

scott-huberty · 2024-07-16T18:09:21Z

Thank you @larsoner ! 🙏

drammock · 2024-07-16T19:05:11Z

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

Reviewed only by eyeballing diff on GH. After these comments are addressed, I can do another review with local testing if requested.

doc/changes/devel/12300.bugfix.rst

mne/io/egi/egi.py

drammock · 2024-07-16T19:19:57Z

mne/io/egi/egi.py

+    if events are not mutually exclusive.

-    This step will fail if events are not mutually exclusive.
+    For these reasons, it is recommended to use ``events_as_annotations=True``.


I recommend reorganizing the Notes section, to put the info in this order:

what happens when events_as_annotations=True (put this first because it's the recommended way, and easier to explain)

what happens when events_as_annotations=False

new stim channel STI014 created (w/ crossref to glossary entry for stim chans)...

...that contains brief (1-sample?) pulses where the Netstation file had event timestamps.

An event_id dictionary is attached (where?) to the Raw object...

...that will have arbitrary sequential integer IDs for the events...

...will fail if any timestamps are duplicated (is that the right way to state the failure mode?)...

...and will not survive a save/load roundtrip.

drammock · 2024-07-16T19:25:00Z

mne/io/egi/egi.py

+        if events_as_annotations:
+            raise NotImplementedError


wait, so if I'm reading this right, events_as_annotations=True only works for .mff (not EGI simple binary fmt)? Docstring needs to say that

drammock · 2024-07-16T19:34:11Z

mne/io/egi/tests/test_egi.py

 egi_pause_w1337_skips = [(21956000.0, 40444000.0), (60936000.0, 89332000.0)]

+# TODO: Remove once complete deprecation / FutureWarning about events_as_annonations
+pytestmark = pytest.mark.filterwarnings("ignore:.*events_as_annotation.*:FutureWarning")


I think maybe this got defined but wasn't used to decorate anything (hence the CI errors)?

Actually it is automagically used in this module

https://docs.pytest.org/en/latest/how-to/skipping.html#skip-all-test-functions-of-a-class-or-module

Just need to add it in test_montage.py (where I'll just decorate the one failing func)

larsoner · 2024-07-16T20:24:42Z

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

scott-huberty added 4 commits December 16, 2023 12:48

ENH: Add option to create annotations from stim channels for EGI

8016baa

STY: rm no-op quotations after black formatting

accbbd1

FIX: remove cruft

537b417

DOC: add docstring for new param

75682d3

larsoner reviewed Dec 19, 2023

View reviewed changes

scott-huberty mentioned this pull request Jan 5, 2024

antisaccade notebook and initial refactor lina-usc/q1k_eeget_init#3

Merged

larsoner added this to the 1.8 milestone Jul 12, 2024

larsoner and others added 5 commits July 16, 2024 10:55

Apply suggestions from code review

4182539

fixed excluding an event that happens first and once.

64f4b18

add name

a24acb2

ENH: Only create if needed

4ffe488

larsoner requested review from agramfort, dengemann and drammock as code owners July 16, 2024 17:16

drammock requested changes Jul 16, 2024

View reviewed changes

FIX: Implement for old version, too

653cecc

larsoner requested review from jasmainak and mscheltienne as code owners July 16, 2024 20:23

drammock approved these changes Jul 16, 2024

View reviewed changes

drammock enabled auto-merge (squash) July 16, 2024 20:38

drammock merged commit fce9c5e into mne-tools:main Jul 16, 2024

Uh oh!

Conversation

scott-huberty commented Dec 16, 2023 • edited by larsoner Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scott-huberty commented Dec 19, 2023

Uh oh!

larsoner commented Jul 15, 2024

Uh oh!

scott-huberty commented Jul 15, 2024

Uh oh!

larsoner commented Jul 16, 2024

Uh oh!

scott-huberty commented Jul 16, 2024

Uh oh!

drammock commented Jul 16, 2024

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

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!

larsoner commented Jul 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

scott-huberty commented Dec 16, 2023 •

edited by larsoner

Loading

larsoner commented Dec 19, 2023 •

edited

Loading