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.

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.

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?

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.

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.

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

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.

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

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.

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.

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?

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.

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:

1. 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.
2. 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"

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.

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

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

That's better.

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?

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…

then maybe logger.info is enough.

Looks good to merge but no rush over the holidays

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

Thanks @alexrockhill !

nice!