Fix EEGLAB events after dropped epochs - #14067
Conversation
| if "epoch_indices" in getfullargspec(eeglabio.epochs.export_set).kwonlyargs: | ||
| kwargs["epoch_indices"] = epochs.selection | ||
| kwargs["epoch_indices"] = np.arange(1, len(epochs) + 1) |
There was a problem hiding this comment.
Is there any reason to pass this at all anymore? I assume if you don't pass it, it defaults to np.arange, no?
There was a problem hiding this comment.
Good question. In most cases, omitting epoch_indices gives the same result. But when zero time is the final sample (tmax=0), eeglabio assigns the event to the next epoch and resets the latencies. I updated the regression test to cover this case; it fails without the explicit indices and passes with them. So I kept the argument and added a short comment.
| events = epochs.events.copy() | ||
| events[:, 0] = ( | ||
| np.arange(len(epochs)) * len(epochs.times) + epochs.time_as_index(0)[0] | ||
| ) |
There was a problem hiding this comment.
This is odd... if you do this there is no longer any reference to the absolute time in the original recording. It suggests events[:, 2] is the only thing correctly used by eeglabio.epochs.export_set. Is it?
There was a problem hiding this comment.
Yes, the absolute sample from the original continuous recording is intentionally not retained in this field. For epoched EEGLAB files, eeglabio uses column 0 as latency in the concatenated epochs array and column 2 as the event type; column 1 is unused. Keeping the original sample numbers is what caused events to point to the wrong epochs after dropping epochs. I clarified this in the code comment.
There was a problem hiding this comment.
Ohhh.... so I think this fix actually belongs in eeglabio. It should handle the translation from MNE-Python events (which it currently takes) to whatever eeglab wants.
There was a problem hiding this comment.
That makes sense. I moved the translation into eeglabio and added tests for dropped epochs and the tmax=0 boundary: jackz314/eeglabio#26. I’ll update this PR once the upstream behavior is settled.
2554ab3 to
5054df6
Compare
|
Okay pushed a commit to simplify the checks after making sure it worked on older and newer EEGLABIO, thanks in advance @viranovskaya ! |
2 participants
Reference issue
Fixes #13535.
What does this implement/fix?
When epochs had been dropped,
epochs.selectionstill referred to the original trials. Passing these indices to eeglabio could produce out-of-range epoch numbers and latencies, followed by dummy events or aneeg_checksetfailure.This change maps events to the epochs that are actually being exported and recalculates their latencies in the concatenated EEGLAB dataset.
I added a regression test with dropped middle epochs. The exported file now contains one event per retained epoch, valid 1-based epoch numbers, and no dummy events.
Event sample numbers after importing the
.setfile back into MNE now refer to the exported epoched dataset rather than the original continuous recording. This matches how EEGLAB represents events in epoched data.Additional information
Tested with eeglabio 0.1.2 and 0.1.3.