Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Annotations for read_raw_egi events #12300

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

scott-huberty
Copy link
Contributor

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?

@@ -103,6 +103,7 @@ def read_raw_egi(
exclude=None,
preload=False,
channel_naming="E%d",
events_as_annotations=False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
events_as_annotations=False,
*,
events_as_annotations=False,

Comment on lines +141 to 146
events_as_annotations : bool
If True, annotations are created from experiment events. If False (default),
synthetic trigger channels are created from experiment events. See the Notes
section for details.

.. versionadded:: 0.14.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
events_as_annotations : bool
If True, annotations are created from experiment events. If False (default),
synthetic trigger channels are created from experiment events. See the Notes
section for details.
.. versionadded:: 0.14.0
.. versionadded:: 0.14.0
events_as_annotations : bool
If True, annotations are created from experiment events. If False (default),
synthetic trigger channels are created from experiment events. See the Notes
section for details.
.. versionadded:: 1.7.0

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Grab the first annotation. Should be the first "DIN1" event.
assert len(raw.annotations)
onset, dur, desc, _ = raw.annotations[0].values()
assert np.isclose(onset, 2.438)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this instead, the error is more informative

Suggested change
assert np.isclose(onset, 2.438)
assert_allclose(onset, 2.438)

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start!

@larsoner
Copy link
Member

larsoner commented Dec 19, 2023

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

@scott-huberty
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: Use annotations for read_raw_egi events rather than synthetic channel
2 participants