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

ENH: Preserve annotations in epochs #8376

Closed
adam2392 opened this issue Oct 15, 2020 · 19 comments · Fixed by #10019
Closed

ENH: Preserve annotations in epochs #8376

adam2392 opened this issue Oct 15, 2020 · 19 comments · Fixed by #10019
Milestone

Comments

@adam2392
Copy link
Member

Describe the new feature or enhancement

Is there a way to map annotations from the raw dataset onto the AverageTFR object? Since the tfr is occurring over windows, then the time scale of the annotations from the original time scale are inherently different.

E.g. if I have an event at sample 10000, with sfreq=1000, then it occurs at 10 seconds into the raw dataset. If i have an averageTFR that was computed with a moving window of say 1 second step size, then the new sample point of the event would be 10.

Describe your proposed implementation

Ideally this would consist of two things that I could see being useful:

  1. A public function map_annotations_to_sfreq that allows mapping Annotations to different time-scales based on a passed say... sfreq value. So based on the above example, if I wanted to map Annotations that are based on sfreq=1000Hz, to some tfr object with sfreq=1 Hz, then I would somehow pass that information to map_annotations_to_sfreq that returns me a new Annotations object that has onset and duration in the new time-scale.

  2. Use the public function above to make AverageTFR, or EpochsTFR, etc. to keep their Annotations when the time-scale changes.

I strongly would prefer having 1 because besides mne-python objects, I create other derivatives of the raw data that inherently have a "sliding" window component to them, and hence change the time-scale in the same way as a sliding FFT would.

Additional comments

@larsoner's comment on gitter for reference:

@adam2392 in principle we should be able to do that at least for EpochsTFR, can you open an issue feature request? For AverageTFR I'm not sure because we don't preserve annotations in Evoked even (just Raw and Epochs) so we'd probably want to do that first before doing it for AverageTFR
@adam2392 adam2392 added the ENH label Oct 15, 2020
@larsoner
Copy link
Member

A public function map_annotations_to_sfreq that allows mapping Annotations to different time-scales

I actually don't think we need this. Annotations store data in seconds not samples -- so they have no notion of (or need for) a sfreq (even if sfreq and sample numbers are used in the Raw instance to create the times in seconds). So I think all we really need to do is ...

Use the public function above to make AverageTFR, or EpochsTFR, etc. to keep their Annotations when the time-scale changes.

... make EpochsTFR (and the functions that create them) respect the annotations properly.

@mmagnuski
Copy link
Member

That would be useful! I would first start with .annotations for Epochs as currently this attribute is only available for raw.
@larsoner although annotations contain time in seconds it might be more user friendly to turn this information into "epoch-centered" times during epoching, so: epoch index, epoch time, duration. Then for TFR we would only need to know the window length for each freq (n_cycles for example, which is not retained, as far as I know, in EpochTFR) and the time or percentage overlap of window with annotation required to make the window annotated.
I'd be happy to help with with this in december, now I don't have much time.

@larsoner
Copy link
Member

larsoner commented Nov 9, 2020

I would first start with .annotations for Epochs as currently this attribute is only available for raw.
@larsoner although annotations contain time in seconds it might be more user friendly to turn this information into "epoch-centered" times during epoching, so: epoch index, epoch time, duration.

Makes sense. We might even be able to do this in a backward-compatible way by adding a epoch_idx attribute to the Annotations class that is None for raw, or a ndarray of int with shape (n_epochs,) when used with an Epochs instance.

As a concrete example, if you have events at sample numbers equivalent to raw t=0 and t=1 and one Raw annotation from 0.25-1.25 sec, if you epoch with tmin=0., tmax=1.you end up with two annotations in epochs, one for epoch index 0 with onset 0.25 and duration 0.75, and one for epoch index 1 with onset 0 and duration 0.25?

@mmagnuski
Copy link
Member

Makes sense. We might even be able to do this in a backward-compatible way by adding a epoch_idx attribute to the Annotations class that is None for raw, or a ndarray of int with shape (n_epochs,) when used with an Epochs instance.

Oh, yes, good idea.

As a concrete example, if you have events at sample numbers equivalent to raw t=0 and t=1 and one Raw annotation from 0.25-1.25 sec, if you epoch with tmin=0., tmax=1.you end up with two annotations in epochs, one for epoch index 0 with onset 0.25 and duration 0.75, and one for epoch index 1 with onset 0 and duration 0.25?

Yes, I think they should be split like this, that would keep the representation simple.

@larsoner larsoner changed the title Mapping Annotations to different time scales ENH: Preserve annotations in epochs Feb 24, 2021
@larsoner
Copy link
Member

Since we have channel-specific annotation support now, I've re-titled this as the missing component, namely what @mmagnuski mentioned:

I would first start with .annotations for Epochs as currently this attribute is only available for raw.

We need to preserve annotations in the Epochs (and EpochsTFR, etc.) objects next.

@adam2392
Copy link
Member Author

What's the status of having epoch-id in events/annotations?

@larsoner
Copy link
Member

Somebody motivated to work on it needs to propose what attribute should be added to Epochs, how it should be saved (maybe reuse FIFFB_MNE_ANNOTATIONS?), and then needs to implement it...

@adam2392
Copy link
Member Author

Epochs are constructed from raw so this should be straightforward to then add to the Annotations with epoch_idx. Then internally, formulate the full Annotations object that also consists of the Epoched events (what's currently stored as events in Epochs). I think this probably could be a set_annotations for Epochs, where it should operate more or less the same as it does with Raw except it checks for epoch_idx.

EpochsTFR is a bit more troublesome because it is formed from the TFR functions. Inside any TFR functions that operate on Epochs data structure, then it can just extract the annotations and do set_annotations on EpochsTFR.

I suppose there might be some complexities with set_annotations functionality for Epochs...

Is there anything internally more difficult? What is FIFFB_MNE_ANNOTATIONS used for?

@adam2392
Copy link
Member Author

Notes from today's discord OH: discuss next week at dev meeting.

Prolly best to only handle Epochs, not EpochsTFR, and replicate annotations for all epochs it falls under and downstream allow users to select which epochs they want to consider, possibly in get_data, or something else?

@drammock
Copy link
Member

I'll add a bit more detail from today's discussion (so I don't forget about it by next week):

  1. some annotations (e.g. for resting state) may be things like "eyes-closed" and may last many seconds / minutes. For such cases we need the annotation entry for that instance of eyes-closed to include every epoch that is fully contained within the annotation, and probably should also include any epochs that are partially contained within it (though it might be good to have that configurable as @adam2392 says).
  2. epochs.crop() will need updating, since it could potentially change which epochs are fully-in, partially-in, or not-in a given annotation.

@adam2392
Copy link
Member Author

Okay I'll take a stab at this based on discord dev meeting:

Annotations for Epochs will be literally the same Annotations as Raw (these are "events" that occur over continuous time). Note the diff vs Epochs.events, which just denote how the "epoching/windowing" occurred over the original raw continuous time. And also metadata, which is data associated with each epoch, not necessarily with some time associated with it

The PR should:

  1. add epochs.set_annotations(annotations) function that can figure out where the Annotations object occurs in the epochs based on using the events array to backtrack how the epoching occurred on the Raw object
  2. add epochs.trim_annotations(), which basically trims annotations (e.g. trims annotations outside the Epochs if you drop "bad" epochs which contain a part of an annotation that is in there). A lot of possibilities here based on what was discussed, so maybe see what's easiest to implement first?
  3. Add something to constructor of Epochs, such as keep_annotations=True to keep the original Raw annotations

cc: @hoechenberger @drammock @larsoner @rob-luke who were on the call

@larsoner
Copy link
Member

I think we'll want some time to fully test + implement this (including viz) so I think this shouldn't go into 0.24 but rather 0.25/1.0

@larsoner larsoner modified the milestones: 0.24, 1.0 Oct 22, 2021
@agramfort
Copy link
Member

agramfort commented Oct 23, 2021 via email

@larsoner
Copy link
Member

This would not work with EpochsArray.

I think we can make it work with EpochsArray. If we make sure that the events sample numbers in the EpochsArray keep the epochs as what would have been separate chunks of time if it had really come from raw so that adding an annotation to the first epoch won't show up in the next epoch. Or are you thinking of some other issue?

@agramfort
Copy link
Member

agramfort commented Oct 23, 2021 via email

@larsoner
Copy link
Member

larsoner commented Oct 24, 2021

with EpochsArray the events onsets are just place older and the meas_date is absent.

meas_date being missing doesn't matter, we have the same problem sometimes for raw. Regarding placeholder -- sure, but I think for the "continuous time" representation to work for EpochsArray under the hood, we just need to make sure that the event onsets are spaced out by more than (tmax - tmin) * sfreq.

So I don't see how annotations would make sense unless you mark them with the GUI and it becomes more meaningful. So maybe....

I think whatever user-facing API you are envisioning for adding annotations to individual epochs (one of these being the GUI example) can/will be the same regardless of whether our "storage backend" for the annotations keeps track of them in the annotations in terms of continuous time, or in terms of epoch-by-epoch annotations separate from the original times. The user won't "feel" our use of a continuous time backend for EpochsArray, but they definitely can for Raw->Epochs->Raw annotations round-tripping for example (I mean this in a good way!).

What also worries me is the use change of sfreq eg if you resample the epochs you lose the provenance of sfreq don't you think?

We should probably adjust the events[:, 0] sample times when resampling and decimating, or alternatively (maybe better) store a epochs._raw_sfreq to keep track of the original sample rate. It actually seems like a bug that we don't do either of these. If we do change this behavior either way, then this issue becomes moot because we still can get from events[:, 0] (maybe we also need a epochs._raw_first_samp?) to the original/true human times (what raw Annotations tries to be in reference to, in some way) of the epoch t=0 points.

If you're not convinced by these arguments @agramfort I say we hold off for another couple weeks and hash it out at the next developer meeting. I think iterating there will be faster, and we don't need to do this for 0.24 so waiting a couple weeks isn't a bad idea anyway.

@agramfort
Copy link
Member

agramfort commented Oct 24, 2021 via email

@mmagnuski
Copy link
Member

by the dev meeting you mean the office hours announced on MNE discourse or something else? I'd be interested to join. :)
I'd also add my 2c to the discussion (or my undrestanding of the discussion) here:

if Annotations have the same meaning as for Raw yes we need to link back to a raw object.

I think it would still be very helpful to have epoch annotations even if they would not link back to the raw object. Or if the "linking" was conditional on not changing the sampling rate etc. If I undrestand correctly, the use case for such "linking" would be to be able to move the annotations created in Epochs back to the Raw object. Something similar to raw.set_annotations(epochs.annotations) or raw.add_annotations_from_epochs(epochs)? While this might be useful I don't think it is crucial for the most common (I think) use case of retaining Raw annotations in Epochs and being able to add/modify annotations to Epochs.

My usecase for epoch annotations for example is mimicing some of the functionality I used in fieldtrip - marking artifacts on epochs (semi-automatically for example) and using these for "partial rejection". More concretely when one is interested in both pre and post-stimulus activity it is convenient to mark artifacts as parts of epochs and then when analysing the pre-stimulus activity - first .crop() to prestimulus time range and then reject the epochs that still contain bad annotations; and do the same, but with different .crop() when analysing post-stimulus activity. Another is using very long epochs (rest segments for example, each about 60 seconds long) and not wanting to reject the whole epoch just because it has two-seconds long bad annotation. I would prefer to retain this annotation and be able to ignore it when calculating welch psd for example (I have a not-so-elegant function for this use case here).

Also, if I undrestand last post by @adam2392 the idea is to keep the same Annotations object to work in Raw and Epochs? Maybe I misunderstood but in majority of cases I would want to programmatically add annotations to epochs via .set_annotations(), I would like to specify which epochs (and their ranges) to annotate and not have to reason back to the Raw to get accurate onsets and durations. While it might be nice to be able to pass normal (raw's) Annotations to epoch.set_annotations() I don't think it is necessary.

BTW @adam2392 I think I had a function that maps bad annotations onto tfr (as NaNs) taking into accont the n_cycles of each TFR "pixel". Let me know if you still need it, I may do some digging :)

@drammock
Copy link
Member

by the dev meeting you mean the office hours announced on MNE discourse or something else? I'd be interested to join.

on discord, but separate from the office hours. I didn't find you in a search of the discord users, ping me there and I'll fill you in.

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

Successfully merging a pull request may close this issue.

5 participants