Unexpected drop_log behaviour when subsetting epochs

[Genuster]

Description of the problem

Bad epoch from irrelevant condition isn't marked as "IGNORED" in drop_log.

Expected results

"IGNORED" is added to the irrelevant epochs even when they're bad.

Actual results

For example: I have only two conditions, either 'stim' or 'blank'.
For epochs['stim'].drop_log the output is:
(('IGNORED',), (), ('EEG044',), (), (), ...
While epochs['blank'].drop_log returns:
((), ('IGNORED',), ('EEG044',), ('IGNORED',), ('IGNORED',), ...

Note how the third epoch isn't ignored in either condition.

Additional information

Platform Windows-11-10.0.22631-SP0
Python 3.12.7 (tags/v3.12.7:0b05ead, Oct 1 2024, 03:06:41) [MSC v.1941 64 bit (AMD64)]
Executable c:\Users\gennadiyb\Documents\erp.venv\Scripts\python.exe
CPU Intel64 Family 6 Model 158 Stepping 10, GenuineIntel (12 cores)
Memory 31.9 GB

Core
├☑ mne 1.8.0 (latest release)
├☑ numpy 2.1.2 (OpenBLAS 0.3.27 with 12 threads)
├☑ scipy 1.14.1
└☑ matplotlib 3.9.2 (backend=module://matplotlib_inline.backend_inline)

Numerical (optional)
├☑ pandas 2.2.3
└☐ unavailable sklearn, numba, nibabel, nilearn, dipy, openmeeg, cupy, h5io, h5py

Visualization (optional)
├☑ qtpy 2.4.1 (PyQt6=6.7.1)
├☑ pyqtgraph 0.13.7
├☑ mne-qt-browser 0.6.3
├☑ ipywidgets 8.1.5
└☐ unavailable pyvista, pyvistaqt, vtk, ipympl, trame_client, trame_server, trame_vtk, trame_vuetify

Ecosystem (optional)
└☐ unavailable mne-bids, mne-nirs, mne-features, mne-connectivity, mne-icalabel, mne-bids-pipeline, neo, eeglabio, edfio, mffpy, pybv

[agramfort]

why not, although it's already dropped. Can you explain how you use the drop_log for your analysis? Does it cause a bug for example in plot_drop_log where it would say that "EEG044" is causing a drop for your epoch selection??

…

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

[Genuster]

Can you explain how you use the drop_log for your analysis?

I'm computing mean/std of dropped epochs over subjects. For this I need absolute counts of dropped epochs per subject. Similarly to plot_drop_log, I get it from drop_log:
absolute = len([x for x in drop_log if len(x) if not any(y in ignore for y in x)])
Would've been nice, actually, to have the absolute count as an attribute/property of Epochs.

Does it cause a bug for example in plot_drop_log where it would say that "EEG044" is causing a drop for your epoch selection??

Yes, it actually does. EEG044 appears on both stim and blank plots.
The number of epochs per condition is also wrong because of the bug. I have 160 stims and 40 blanks and not 161 and 41 as in the plots:

All

Stim

Blank

[agramfort]

I am personally enclined to say then that the change could be done as a bug fix

[larsoner]

I think appending 'IGNORED' would make sense in this case, and would be fine as a bug fix. It adds potentially useful information, and seems like you can calculate whatever you want from it with a and "IGNORED" not in x or whatever. I wouldn't want to replace "EEG 044" because that's backward incompatible and I imagine could break some people's existing calculations, etc.

[Genuster]

I've looked through the code and it seems that the info about original events and event_id is completely lost when dropping epochs (BaseEpochs.drop_bad removes the entries from events and event_id through BaseEpochs._get_data through GetEpochsMixin._getitem). I can think of two (not so pretty) solutions:

1. More general solution: Store the original events and ids when the Epochs instance first created and pass them to all modified versions (e.g. after dropping bads or subsetting). But this will probably involve some complication with epochs IO (?)
2. More specific solution: Store the event string and id alongside channel names in drop_log, something like that: (('Ch1', 'Ch2', ('a/b/c', 9)),(),()...).

What do you think @agramfort, @larsoner?

[larsoner]

1. ... But this will probably involve some complication with epochs IO (?)

Looks like we currently use FIFF_MNE_EVENT_LIST to store events. There is a FIFF_EVENT_LIST we could use instead to store the "current epochs.events", and then use FIFF_MNE_EVENT_LIST to store the original events (and if only FIFF_MNE_EVENT_LIST is present, which would be the case for all files up until now, know that that is the "current epochs.events"). And we currently store the event_id in FIFF_DESCRIPTION and we could store the original ones in FIFF_MNE_EVENT_COMMENTS. So it's doable in principle I think. Seems like a lot of extra work and complication, though, so would be nice if we could get away without it! Not sure if there is another way, though.

Store the event string and id alongside channel names in drop_log, something like that: (('Ch1', 'Ch2', ('a/b/c', 9)),(),()...).

I don't love this because now it's mixing concepts in drop_log -- previously/currently it's always "reasons why ignored" and with this it would become "reasons why ignored or original event number", which isn't as clear/clean.