mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

ENH: Remaining stuff for Epochs annotations

Open larsoner opened this issue 2 years ago • 11 comments

From https://github.com/mne-tools/mne-python/pull/9969, @adam2392 we should take care of these before 1.0. Creating this issue so that we don't forget:

  • [x] Annotations will be lost on save. (#10019)
  • [ ] It is not yet possible to add annotations to the Epochs object programmatically (via code)

The first one here seems less critical to me, so I think we could leave it for 1.1, and the second one is too hard for 1.0:

  • [ ] Concatenating :class:~mne.Epochs objects that contain annotations is not supported, and any annotations will be dropped when concatenating.
  • [ ] It is not yet possible to add annotations to the Epochs object interactively (through the plot window)

larsoner avatar Dec 17 '21 17:12 larsoner

  • [ ] Annotations will be lost on save.

Putting up PR in #10019

  • [ ] It is not yet possible to add annotations to the Epochs object programmatically (via code)

Will think about this a bit. This is going to be the set_annotations function that will be added to Epochs.

  • [ ] Concatenating :class:~mne.Epochs objects that contain annotations is not supported, and any annotations will be dropped when concatenating.

I have a few feasibility questions on this. If Epochs are created from completely different data files, then their continuous time relative to each other will be lost. The only way I could see this being bypassed is utilizing a datetime for the original time for each Epochs object.

adam2392 avatar Dec 17 '21 17:12 adam2392

If Epochs are created from completely different data files, then their continuous time relative to each other will be lost. The only way I could see this being bypassed is utilizing a datetime for the original time for each Epochs object.

Yes I think what we would want to do is take the first epoch object's annotations as the "real actual time", then make the annotations for all the other concatenated epochs show up in the right places. Something like this...

larsoner avatar Dec 17 '21 17:12 larsoner

This is going to be the set_annotations function that will be added to Epochs.

I think we'll want set_annotations to only take an Annotations instance. So really we need something different, like set_annotations_by_epoch or something, which would round-trip with get_annotations_by_epoch or whatever you called it in #9969 (in the simple case that annotations are exactly contained within a single epoch, and no epochs overlap at least).

larsoner avatar Dec 17 '21 17:12 larsoner

From #9969, @adam2392 we should take care of these before 1.0. Creating this issue so that we don't forget:

  • [ ] Annotations will be lost on save.
  • [ ] It is not yet possible to add annotations to the Epochs object programmatically (via code)

With #10019 both will be achieved.

adam2392 avatar Jan 04 '22 15:01 adam2392

Hi, thanks @adam2392 for working on this, it's great to have epoch-level annotations. I updated the first post by Eric to reflect that the first todo box can be marked. The second one still remains to be ticked off, I think. There is .set_annotations() for epochs but the annotations passed to it should be timed wrt the raw file. It would be nice to be able to set the annotations specifying epoch indices and times wrt to epoch times. But please correct me if I'm wrong.

mmagnuski avatar Feb 06 '22 13:02 mmagnuski

@mmagnuski Thanks glad you find it useful!

I'm not sure I follow. Would you be able to illustrate an example for that use case? I think @drammock @larsoner or @agramfort might have some thoughts.

adam2392 avatar Feb 11 '22 16:02 adam2392

It would be nice to be able to set the annotations specifying epoch indices and times wrt to epoch times. But please correct me if I'm wrong.

Would you be able to illustrate an example for that use case?

A simple one is if someone does read_epochs_eeglab or creates an EpochsArray -- it would be nice for them to be able to say "I know in epoch 2 in this time interval something bad happens", both via interaction in the epochs browser, and programmatically via some suitable method like epochs.set_annotations_by_epoch(...) (name and API TBD)

larsoner avatar Feb 11 '22 17:02 larsoner

I see that makes sense. Then set_annotations_by_epoch would set one "annotation" with the duration of the epoch by default. Alternatively, it can set multiple annotations with no duration.

I am down to work on this down the line, but am quite busy recently. Maybe a newcomer developer would like to take this on?

Seems like a straightforward handling of events data structures and the Epochs class.

adam2392 avatar Feb 11 '22 21:02 adam2392

I see that makes sense. Then set_annotations_by_epoch would set one "annotation" with the duration of the epoch by default. Alternatively, it can set multiple annotations with no duration.

Now we're talking API.. I think maybe a user could pass a list of length n_epochs with onset/duration/desc tuples inside or something. But we can hash that out based on a use case...

I am down to work on this down the line, but am quite busy recently. Maybe a newcomer developer would like to take this on?

... so I think we should wait until someone with a sense of using MNE Epochs and Annotations wants to actually do this sort of thing, and let that guide the API. Having an actual use case will make it clear what we need to add, I think.

larsoner avatar Feb 11 '22 23:02 larsoner

Some suggestions API-wise from how I would like to use such function:

# mark whole epochs with annotations providing only epoch indices and description:
epochs.set_annotations_by_epoch([4, 6, 15, 35], description='bad_blink_at_stimulus_onset')

# mark specific regions per epoch:
epochs.set_annotations_by_epoch([12, 40], onset=[-0.2, 0.45], duration=[0.43, 0.3],
                                description='oscillatory_burst')

this second way of setting epoch level annotations might be especially useful for automatically marking various sub-epoch events - like the oscillatory bursts in the example above.

mmagnuski avatar Feb 12 '22 12:02 mmagnuski

Looks like a reasonable API to me!

larsoner avatar Feb 12 '22 13:02 larsoner