Annotations metadata

[PierreGtch]

Reference issue (if any)

Closes #13199

What does this implement/fix?

This PR adds a metadata attribute to the Annotation objects.
I saw that #12213 was initially going in a similar direction but did not implement it in the end, not sure why.

Additional information

As mentioned in #13199, this feature is relevant when we need more information to describe the events than just timing and name.

[welcome]

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

[PierreGtch]

@matthiasdold this should help with mne-tools/mne-bids#1389 and NeuroTechX/moabb#744 :)

[PierreGtch]

@sappelhoff what do you think about this implementation?

And do you know what the issue is with the "Ultraslow_PG" test? The log is quite cryptic...

[sappelhoff]

Hey Pierre, I will not have time to review this in the next days, unfortunately. I will come back to it if I can.

And do you know what the issue is with the "Ultraslow_PG" test? The log is quite cryptic...

I think the error is unrelated to your changes as it fails already in the installation step

[PierreGtch]

I don't understand why mne/commands/tests/test_commands.py::test_show_fiff fails in the CI. It works locally

[drammock]

I saw that #12213 was initially going in a similar direction but did not implement it in the end, not sure why.

It would be good if @britta-wstnr and/or @eioe could weigh in here, based on this comment. Sadly I don't recall the details of that conversation, but maybe one of them does!

@PierreGtch one specific question I have is why it is necessary for the metadata attached to an annotation to be a dataframe rather than a dict --- if the goal is "populate epochs.metadata when we create epochs from annotations" then wouldn't you be creating 1 row per epoch, AKA 1 row per annotation? And thus wouldn't you only need a column_name: value mapping? More generally, it would help me if you could explain in a bit more detail why this is needed / what case it can handle that is either hard or impossible with make_metadata().

[eioe]

Hi, I do not recall the discussion en detail as well, but I think the most relevant arguments are outlined in #12208 .

[PierreGtch]

@PierreGtch one specific question I have is why it is necessary for the metadata attached to an annotation to be a dataframe rather than a dict

@drammock it is not necessary for the annotations.metadata to be a dataframe. I started the PR using a dataframe to follow the implementation of epoch.metadata.
annotations.metadata could indeed be a dict[str, Sequence[Any]].
...or even a Sequence[dict[str, Any] | None] because not all annotation rows will require the same extra columns (for example the start of an experiment vs. an eye tracking event)

More generally, it would help me if you could explain in a bit more detail why this is needed / what case it can handle that is either hard or impossible with make_metadata().

In mne-bids, when reading raw recordings with events in an associated _events.tsv file, only the onset, description and duration columns are read and stored in the raw.annotations. All other columns of the _events.tsv file are discarded, and you need to load them manually and add them manually to the epochs.metadata columns. If in-between some bad epochs were dropped or only certain event types were selected, things become messy...
It would simplify everything if we were able to load all the _events.tsv columns in the raw.annotations.

[drammock]

If in-between some bad epochs were dropped or only certain event types were selected, things become messy...

on this point, epochs.selection attribute can help. But I see how the process could be error-prone and could be automated. Thanks for explaining.

I think the most relevant arguments are outlined in #12208 .

Wow, I'd forgotten about all of that back-and-forth. @PierreGtch now that I've been reminded of all the past thinking / prototyping we did on this issue, WDYT about following the design described in #12208 (comment) and #12208 (comment) ?

[PierreGtch]

WDYT about following the design described in #12208 (comment) and #12208 (comment) ?

@drammock Yes, a list of dict seems like a perfect option!

Regarding the name of the attribute, like @eioe, I now prefer details. But data is also fine.

And to prevent users from editing in place the list or dicts, we could return a deep copy in the property. WDYT?

[drammock]

to prevent users from editing in place the list or dicts, we could return a deep copy in the property. WDYT?

That would indeed preserve the fidelity of the attached data. But it wouldn't prevent users from trying to edit the data and thinking that it worked and then being confused when the data they manually added is mysteriously not there. That's not a good UX.

[PierreGtch]

Closing in favour of #13228, which follows the recommendations in this #13213 (comment)