Replace metadata eval with query

[cbrnr]

Fixes #10700. Let's see if all tests still pass.

[agramfort]

seams to work @cbrnr !

can you add a test with your toy example

[cbrnr]

OK, but we might have a problem here, because what should happen if an event_id has the same name as a metainfo column?

import mne
import numpy as np
from pandas import DataFrame, Series, NA

metadata = DataFrame(
    {"A": Series([True, True, True, False, False, NA], dtype="boolean")}
)
rng = np.random.default_rng()
epochs = mne.EpochsArray(
    data=rng.standard_normal(size=(6, 8, 500)),
    info=mne.create_info(8, 250, "eeg"),
    event_id={"A": 1},  # event ID "A" has the same name as metainfo column "A"
    metadata=metadata
)

# ambiguous: should epochs["A"] get epochs of type "A" or where metainfo["A"] matches?
assert len(epochs["A"]) == 3  # gets epochs for which column A == True

assert len(epochs["not A"]) == 2  # gets epochs for which column A == True
assert len(epochs["A.isna()"]) == 1  # gets epochs for NA in column A

[drammock]

what should happen if an event_id has the same name as a metainfo column?

We should clearly document which one has precedence. IIRC in the code we try event_id first so epochs['A'] should get epochs of type A, not epochs for which metadata column A is truthy. (anyway that's how I think it ought to be). If we want to be really nice we could detect such conflicts and print an info or warning when they occur.

[hoechenberger]

Ohohoh, I'd like to ensure this doesn't break the MNE-BIDS-Pipeline before we merge this

[mmagnuski]

@cbrnr @drammock
I also think event_id should have precedence. For boolean columns I'd generally recommend the slightly more verbose query "A == True" to avoid such conflicts.

[cbrnr]

@agramfort I was going to write a bit more compact test and haven't pushed it yet. Do you want to keep the test you added?

[agramfort]

feel free to adjust my test no pb

…

Message ID: ***@***.***>

[cbrnr]

Alright, so if there is an event_id labeled A, epochs['A'] will continue to get epochs with this ID (so the previous behavior does not change), even if there is also a metadata column A. If that column A is boolean, we can query all rows where this column is True with epochs["A == True"]. If there is no metadata column with the same name, then epochs['A'] will get all epochs where this column is True.

So I guess this is already what we want?

[hoechenberger]

I have to say I'd like to avoid us having to make this kind of decision in the first place. Can we not raise a ValueError if the user tries to set metadata with column names equal to an event name?

Or, allow me to go crazy here ... do everything via metadata internally, as in always create metadata with columns event_id and event_name.... 🤔

[agramfort]

Or, allow me to go crazy here ... do *everything* via metadata internally, as in always create metadata with columns event_id and event_name.... Message ID: <mne-tools/mne-python/pull/10705/c1148439208@ github.com>

this means making pandas a hard dependency of mne as Epochs are everywhere

[cbrnr]

Building everything upon metadata would be cool, but this will be a huge change. It's probably not worth it unless someone really wants to do it.

In any case, @hoechenberger you said you wanted to verify that everything still works with BIDS – did you have a chance to do that? We should wait for you before we merge this PR.

[hoechenberger]

In any case, @hoechenberger you said you wanted to verify that everything still works with BIDS – did you have a chance to do that? We should wait for you before we merge this PR.

Nope, will want to do that later today!!

[hoechenberger]

Seems to be working with the pipeline, hence all good from my end!

[hoechenberger]

Thanks @cbrnr and @agramfort!

[mmagnuski]

🎉