[ENH, MRG] Allow epoch construction from annotations #12311

alexrockhill · 2023-12-19T07:02:12Z

With the advent of annotations and their widespread use, it seems like events could be abstracted from the user unless they want to handle time stamps directly. Especially with annotations being assigned directly from BIDS datasets through mne-bids it seems to me that the conversion to events becomes superfluous to the user. So it seems reasonable to me to use the annotations by default when constructing epochs. Thoughts?

hoechenberger · 2023-12-19T08:31:15Z

Thoughts?

Yes! Yes! Yes!!!

I hate having to convert my BIDS annotations to events all the time. It's just so unnceseccary. +1000 for allowing Epochs creation from Annotations.

hoechenberger · 2023-12-19T08:33:58Z

mne/epochs.py

+        if events is None:
+            events, event_id_tmp = events_from_annotations(raw)
+            if events.size == 0:
+                raise RuntimeError("No events found in raw.annotations")


This message is a little bit of a misomer, no?

Also how can I constrain the annotations that will be used to construct epochs?

Or would your idea be to select the desired subset of epochs after creation?

Are you thinking no onsets found?

I was thinking you could not preload so then it would be no big deal to select after creation but before load_data.

Are you thinking no onsets found?

No, more like, "no annotations found" ;)

I was thinking you could not preload so then it would be no big deal to select after creation but before load_data.

Fair enough!

I changed this to allow event_id to match the description for constraining after looking through examples in the documentation, I think this is pretty elegant.

mscheltienne · 2023-12-19T10:16:50Z

Sounds good to me, and if people want to control which annotation is used to create the events/epochs, they can fall back to events_from_annotation.

hoechenberger · 2023-12-19T10:28:11Z

Sounds good to me, and if people want to control which annotation is used to create the events/epochs, they can fall back to events_from_annotation.

I was thinking about accepting Annotations in place of events; but this may open a can of worms where the onset times don't match up, right? Or is there a safe, elegant way to resolve this?

cbrnr · 2023-12-19T11:50:35Z

Why not add a new annotations parameter that can be used instead of events? To keep things simple, only one of the two can be used, not both.

hoechenberger · 2023-12-19T12:40:59Z

I forgot that we have to pass Raw in any case. So I suppose it's okay to expect the annotations to be glued to that raw. I wouldn't add a new parameter.

mscheltienne · 2023-12-19T14:29:09Z

-1 to add a new parameter and/or to pass Annotations separately from raw. IMO, if someone needs control over which annotations are included to create the epochs, they should fall back to events_from_annotation. The question is, does this represent a minority or a majority of the use cases?
Personally, I rarely store events as annotations and always use the events array.

hoechenberger · 2023-12-19T14:53:39Z

Personally, I rarely store events as annotations and always use the events array.

I suppose you're working with MEG data though. All of our EEG readers create annotations, not events.

And MNE-BIDS creates annotations, not events.

It's really events these days that are the odd ones, not annotations.

larsoner · 2023-12-19T15:11:08Z

-1 on new annotations parameter, +1 on what @alexrockhill has already (use raw.annotations). No object to events=None doing this.

@alexrockhill can you look to see if any of our existing examples could be improved by this?

mscheltienne · 2023-12-19T15:21:34Z

I suppose you're working with MEG data though. All of our EEG readers create annotations, not events.

Both, with EEG, we usually have a stim channel which behave similarly to MEGIN's STI 101 or STI 102 channels. Thus I end up using find_events most of the time.

drammock · 2023-12-19T15:34:49Z

with annotations being assigned directly from BIDS datasets through mne-bids it seems to me that the conversion to events becomes superfluous to the user. So it seems reasonable to me to use the annotations by default when constructing epochs.

I agree with this reasoning. My main hesitation is that annotations have a duration, and IIUC the behavior here will simply ignore the duration of annotations, and construct tmin,tmax epochs around the onsets of all annotation spans present. That feels unintuitive/unexpected so I think there needs to be prominent messaging/documentation about this.

hoechenberger · 2023-12-19T16:26:53Z

I agree with this reasoning. My main hesitation is that annotations have a duration, and IIUC the behavior here will simply ignore the duration of annotations, and construct tmin,tmax epochs around the onsets of all annotation spans present. That feels unintuitive/unexpected so I think there needs to be prominent messaging/documentation about this.

Interesting thinking! Are you saying you'd expect that epochs should be created such that they contain an entire annotation?

Most Annotations I encounter in BIDS country :) are actually without duration, but I absolutely see your point!

Maybe a warning or info message could make sense?

alexrockhill · 2023-12-19T16:35:16Z

I agree with this reasoning. My main hesitation is that annotations have a duration, and IIUC the behavior here will simply ignore the duration of annotations, and construct tmin,tmax epochs around the onsets of all annotation spans present. That feels unintuitive/unexpected so I think there needs to be prominent messaging/documentation about this.

That seems reasonable to document and maybe warn. I think documenting is an easy yes. I am hesitant to do too many warnings that just cause output-reading-fatigue. Would it be reasonable to check if the durations are different and warn only in that case? Or only if they are different and exceed the window of tmin to tmax? Or just the latter?

I suppose we could infer a more reasonable tmin/tmax from the duration attribute of the annotations but I think this gets into the theoretical/design question in #3533 that Jean-Remi King answered by saying that Epochs themselves are non-ragged matrices of comparable trials. So, if we're following that, it seems to me that the explanation to the user would be Epochs are what you use when you have trials of the same duration and you use something else (TBD) or another function that acts on raw as in #12315 if you want to do variable-length analysis.

drammock · 2023-12-19T20:16:33Z

Are you saying you'd expect that epochs should be created such that they contain an entire annotation?

personally no, I don't expect that, but that's because I know how Epochs and events_from_annotations work 🙂. I'm putting myself in the shoes of someone looking at a Raw plot with annotation spans in it, and then running Epochs(raw) and getting epochs that start earlier than the annotations do (default is tmin=-0.2) and end somewhere almost guaranteed not to coincide with annotation offset --- and not having an easy way to plot the epoch spans alongside the annot spans in order to notice the discrepancy. Especially if that user doesn't have a strong mental model that "epochs must have equal length" (see below), it seems almost guaranteed that someone will make that mistake if what they understand is "pass events=None to make the epochs from the existing annotations".

(I agree that for zero-length annotations the conceptual gap isn't really there.)

As for what to do:

if we did have support for variable-length epochs, then I can imagine users wanting to extract epochs that coincided with the annotations' onsets/offsets (possibly with the ability to expand the time window around each annotation). Since variable-length epochs recently came up again in [ENH] PSD of Ragged Epochs #12315, I think it's worth considering now how that use case might be supported here, while we're contemplating changes to the Epochs API. It's such a central part of our software; if we're going to change two somethings about it, better to do them both at once. EDIT: I'm not saying that "annots-as-events" must be in the same PR as "ragged-epochs"; I'm saying that we should plan the API changes that are needed for ragged-epochs --- especially w/r/t using annotations-as-events --- and put (some of) those API elements in place in this PR, then later we can actually add the ragged functionality.
For the existing (fixed-length epochs) case, I think a logger.info (plus a prominent admonition in the docstring) is probably sufficient for the case of "you asked for annotation-based epoching but your annotations have non-zero duration, heads up that annot duration is being ignored"

alexrockhill · 2023-12-19T20:28:26Z

Ok, I think that is very reasonable to make this into manageable chunks. I will add the logger.info. Since duration is a required attribute of annotations, I think that makes a lot of sense until/if variable length epochs are supported.

alexrockhill · 2023-12-19T22:07:23Z

@hoechenberger 's enthusiasm was very motivating so I went through all the examples that use mne.events_from_annotations and I think this is a nice improvement in brevity and clarity without being incompatible with any of the existing documented use-cases. It's nicely saves the round trip on all the examples that don't deal with the events directly. For the variable length EEGLAB example and for creating metadata where you have to dig into the events, it's not appropriate but for most use cases, it looks like just the ticket.

Going through the examples made me realize we should allow event_id to be a str or list of str as well to mirror the acceptance of int and list of int when events are passed. If events are passed, and you pass a string, you get an informative type error that event_id must be int. But then if you read the docstring for event_id, you see you can pass str if you use events=None. Then, if you pass int event_ids using annotations you'll get an error that the event_id is not found in the annotations. I think this has a nice separation of the event-based epochs and annotation-based epochs where in the former you would use int event_id and in the latter you would naturally use str. I think in this way there will be minimal confusion and informative errors.

alexrockhill · 2023-12-21T19:15:25Z

Dang it, I didn't fix the events in that note that should be annotations

alexrockhill · 2023-12-21T19:54:27Z

https://output.circle-artifacts.com/output/job/13118c23-76b6-4944-ac23-535cc2ee44ab/artifacts/0/html/generated/mne.Epochs.html#mne-epochs

That's better.

drammock

very nice! just a few suggestions, all are minor and are trying to improve clarity of docstrings / messages / code.

@larsoner do you want a look?

doc/changes/devel/12311.newfeature.rst

mne/epochs.py

mne/tests/test_epochs.py

for more information, see https://pre-commit.ci

…nto epochs

agramfort

in Annotations the duration tells you if it's an instantaneous event (so basically just an event as historically named). Would it make sense to warn if you get epochs from events with duration > 0? just thinking at loud...

agramfort · 2023-12-22T08:40:56Z

mne/epochs.py

@@ -3249,6 +3260,38 @@ def __init__(
        # keep track of original sfreq (needed for annotations)
        raw_sfreq = raw.info["sfreq"]

+        # get events from annotations if no events given
+        if events is None:


I would put this block in a private function to be clear about what it needs and returns

+1, the __init__ is already long enough as it is and a separate function would help with that

drammock · 2023-12-22T11:06:44Z

Would it make sense to warn if you get epochs from events with duration > 0?

There's a logger.info in that case. Do you feel strongly that it should be a warning?

hoechenberger · 2023-12-22T11:07:04Z

in Annotations the duration tells you if it's an instantaneous event (so basically just an event as historically named). Would it make sense to warn if you get epochs from events with duration > 0? just thinking at loud...

In my data I use both:
the EEG amplifier only adds instantaneous events, which MNE then imports as annotations with zero duration. But I usually modify the annotations and add durations e.g. for stimulus presentation before writing the data to BIDS. When reading it back and creating epochs, I will have a mixture of both, annotations with zero and non-zero durations. I don't think I should get a warning in this case…

agramfort · 2023-12-22T14:48:56Z

then maybe logger.info is enough.

larsoner

Looks fairly simple and indeed makes examples cleaner! Just one idea for simplification/unification

larsoner · 2023-12-22T18:47:48Z

mne/epochs.py

@@ -3249,6 +3260,38 @@ def __init__(
        # keep track of original sfreq (needed for annotations)
        raw_sfreq = raw.info["sfreq"]

+        # get events from annotations if no events given
+        if events is None:


+1, the __init__ is already long enough as it is and a separate function would help with that

larsoner · 2023-12-22T18:51:33Z

mne/epochs.py

+            if isinstance(event_id, (list, tuple, set)):
+                if set(event_id).issubset(set(event_id_tmp)):
+                    event_id = {my_id: event_id_tmp[my_id] for my_id in event_id}
+                    # remove any non-selected annotations
+                    annotations.delete(
+                        ~np.isin(raw.annotations.description, list(event_id))
+                    )
+                else:
+                    raise RuntimeError(
+                        f"event_id(s) {set(event_id) - set(event_id_tmp)} "
+                        "not found in annotations"
+                    )


The logic here should probably mimic what we do already for missing event IDs, e.g.,. having Epochs(raw, event_id=["auditory", "visual"], on_missing="ignore") should be okay when there are only "auditory" events in the annotations. Maybe you can remove some of the logic here to allow the existing event_id+on_missing checking/logic to take care of some stuff? (And please add a test for this case!)

It looks like this comment was not addressed yet and was errantly resolved by GitHub because the code was moved

Ahh I see there is an _on_missing in there now, never mind! Maybe we could deduplicate more at some point (?) but seems okay for now

alexrockhill · 2023-12-27T18:08:28Z

Looks good to merge but no rush over the holidays

alexrockhill · 2023-12-30T16:56:36Z

Now there is a fix to suppress a deprecation warning for Circle and an xvfb dependency for Qt testing so probably makes sense to merge this or split those out

larsoner · 2023-12-31T02:35:11Z

Thanks @alexrockhill !

hoechenberger · 2023-12-31T09:43:13Z

nice!

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

[ENH] Allow epoch construction from annotations

4fa044e

alexrockhill requested review from drammock, agramfort, mscheltienne and dengemann as code owners December 19, 2023 07:02

alexrockhill marked this pull request as draft December 19, 2023 07:02

ruff

eb73298

hoechenberger reviewed Dec 19, 2023

View reviewed changes

larsoner mentioned this pull request Dec 19, 2023

[ENH] PSD of Ragged Epochs #12315

Draft

go through all examples using events_from_annotations

daee067

alexrockhill changed the title ~~[ENH] Allow epoch construction from annotations~~ [ENH, MRG] Allow epoch construction from annotations Dec 19, 2023

ruff

4447ed8

alexrockhill marked this pull request as ready for review December 19, 2023 21:58

alexrockhill requested a review from larsoner as a code owner December 19, 2023 21:58

Merge branch 'main' into epochs

24270d1

alexrockhill added 2 commits December 21, 2023 11:16

events->annotations

5b9e571

Merge branch 'main' into epochs

517c789

drammock approved these changes Dec 21, 2023

View reviewed changes

alexrockhill and others added 8 commits December 21, 2023 15:29

Dan review

1290aac

[pre-commit.ci] auto fixes from pre-commit.com hooks

4c46e11

for more information, see https://pre-commit.ci

style

27615c8

Merge branch 'main' into epochs

7db0b59

Merge branch 'epochs' of https://github.com/alexrockhill/mne-python i…

fb87172

…nto epochs

space

7ef087d

fix outdated example

c3ae450

style

f9401bd

agramfort reviewed Dec 22, 2023

View reviewed changes

drammock mentioned this pull request Dec 22, 2023

Add tolerance parameter to events_from_annotations to #12321

Closed

larsoner reviewed Dec 22, 2023

View reviewed changes

review

cad31f4

hoechenberger and others added 3 commits December 27, 2023 20:55

Merge branch 'main' into epochs

2c33dab

ignore warning

52542ea

fix xvfb

135f523

larsoner merged commit c73b8af into mne-tools:main Dec 31, 2023
29 checks passed

alexrockhill deleted the epochs branch December 31, 2023 06:11

[ENH, MRG] Allow epoch construction from annotations #12311

[ENH, MRG] Allow epoch construction from annotations #12311

Conversation

alexrockhill commented Dec 19, 2023

hoechenberger commented Dec 19, 2023

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

mscheltienne commented Dec 19, 2023

hoechenberger commented Dec 19, 2023 • edited

cbrnr commented Dec 19, 2023

hoechenberger commented Dec 19, 2023 • edited

mscheltienne commented Dec 19, 2023

hoechenberger commented Dec 19, 2023 • edited

larsoner commented Dec 19, 2023

mscheltienne commented Dec 19, 2023

drammock commented Dec 19, 2023

hoechenberger commented Dec 19, 2023

alexrockhill commented Dec 19, 2023

drammock commented Dec 19, 2023 • edited

alexrockhill commented Dec 19, 2023

alexrockhill commented Dec 19, 2023 • edited

alexrockhill commented Dec 21, 2023

alexrockhill commented Dec 21, 2023

drammock left a comment

Choose a reason for hiding this comment

agramfort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drammock commented Dec 22, 2023

hoechenberger commented Dec 22, 2023 • edited

agramfort commented Dec 22, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexrockhill commented Dec 27, 2023

alexrockhill commented Dec 30, 2023

larsoner commented Dec 31, 2023

hoechenberger commented Dec 31, 2023

hoechenberger commented Dec 19, 2023 •

edited

hoechenberger commented Dec 19, 2023 •

edited

hoechenberger commented Dec 19, 2023 •

edited

drammock commented Dec 19, 2023 •

edited

alexrockhill commented Dec 19, 2023 •

edited

hoechenberger commented Dec 22, 2023 •

edited