Skip to content

Handle special cases from MNE-Python#18

Merged
larsoner merged 10 commits intojackz314:mainfrom
scott-huberty:export_epochs
Sep 24, 2025
Merged

Handle special cases from MNE-Python#18
larsoner merged 10 commits intojackz314:mainfrom
scott-huberty:export_epochs

Conversation

@scott-huberty
Copy link
Copy Markdown
Contributor

@scott-huberty scott-huberty commented Sep 22, 2025

This is a WIP. I am trying to get an MNE-Python test passing again:

https://github.com/mne-tools/mne-python/blob/936bfae75fc4604443e5dc3e16c095cd9bd612a2/mne/export/tests/test_export.py#L539

When using export_set, I think we hit two special cases with MNE-Python, one of which particularly comes from the sample data MNE.datasets.sample.data_path

  1. When you load that dataset, raw.first_samp is something like 25,800. So all event latencies are offset from that number.
  2. If an MNE-Python user drops bad epochs before calling ‘get_data’, then the epochs dimension of the array will not account for the true number of trials.

AFAICT we need to account for these two occurences by allowing ther user to pass in a fist_samp and n_trials value to export_set

I am trying to get an MNE-Python test passing again: https://github.com/mne-tools/mne-python/blob/936bfae75fc4604443e5dc3e16c095cd9bd612a2/mne/export/tests/test_export.py#L539

I think we hit two special cases with the MNE-Python, and one of which particularly comes from the  sample data. 1) When you load that dataset, `raw.first_samp` is something like 25,800. So all event latencies are offset from that number. 2) If an MNE-Python user drops bad epochs before getting their data, then the epochs dimension of the array will not account for the true number of trials. AFAICT we need to account for these two occurences by allowing ther user to pass in a fist_samp and n_trials value to export_set
This reverts commit 865349e AND 57155a4

Now that I've sat with this more I don't think adding a first_samp and n_trials will do the job
I'm not sure that this is the right way to go but our test will pass now so we can go from here...
scott-huberty added a commit to scott-huberty/mne-python that referenced this pull request Sep 23, 2025
Hand in Hand with jackz314/eeglabio#18

I am not sure if this is the right way to go but at least our test will pass now..
@scott-huberty
Copy link
Copy Markdown
Contributor Author

scott-huberty commented Sep 24, 2025

I took a second pass at this, using a different approach this time.

But at least if you run the code below with MNE-Python pointing to my branch (https://github.com/scott-huberty/mne-python.git@test_export_epocsh_eeeglab), it will pass, and it will fail on main, because the event onset latencies change across an IO trip.

The bug arises because eeglabio tries to infer epoch indices from event onsets in sample space. This only works if epochs are contiguous and complete.

The fix here is to allow the user to pass in epoch indices explicitly, then eeglabio can just do a 1-1 mapping between the events_array and epochs_indice array, so the correct event onsets are preserved across an IO trip.

from pathlib import Path
import tempfile

from eeglabio.epochs import export_set
import mne

from numpy.testing import assert_allclose

from pymatreader import read_mat

test_data_dir = Path(mne.__file__).parent / "io" / "tests" / "data"
test_data_fpath = test_data_dir / "test_raw.fif"
events_fpath = test_data_dir / "test-eve.fif"
raw = mne.io.read_raw_fif(test_data_fpath, preload=True).pick("eeg")
events = mne.read_events(events_fpath)

raw.resample(600, events=events)
# let's drop one epoch to prove the point
bad_annot = mne.Annotations(onset=[4.4], duration=[0.1], description=["BAD_manual"])
raw.set_annotations(raw.annotations + bad_annot)
epochs = mne.Epochs(raw, events, preload=True)
data = epochs.get_data()

print(f"Epoch Indices: {epochs.selection}")

want_latencies = epochs.events[:, 0]
with tempfile.TemporaryDirectory() as tmpdir:
    export_set(
        fname=f"{tmpdir}/foo.set",
        data=data,
        sfreq=epochs.info["sfreq"],
        events=epochs.events,
        event_id=epochs.event_id,
        tmin=epochs.tmin,
        tmax=epochs.tmax,
        ch_names=epochs.ch_names,
        epoch_indices=epochs.selection,  # <--- Remove this arg if calling from Main
    )
    epochs_in = mne.read_epochs_eeglab(
        f"{tmpdir}/foo.set", events=epochs.events, event_id=epochs.event_id
        )
    mat = read_mat(f"{tmpdir}/foo.set")  # If you want it
    epochs_in = mne.read_epochs_eeglab(f"{tmpdir}/foo.set")
    got_latencies = epochs_in.events[:, 0]
    assert_allclose(want_latencies, got_latencies)

It seems like eeglabio is trying to infer epoch indices from event onsets (in sample space), but if the epochs are discontinuous (e.g. bad epochs dropped), this sample onset -> epoch indices mapping is not guaranteed.

@larsoner
Copy link
Copy Markdown
Collaborator

It seems like eeglabio is trying to infer epoch indices from event onsets (in sample space), but if the epochs are discontinuous (e.g. bad epochs dropped), this sample onset -> epoch indices mapping is not guaranteed.

This seems like reasonable default behavior, and also reasonable to allow passing epoch_indices as a fix

Copy link
Copy Markdown
Collaborator

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this good to go other than the TODO in the code?

Copy link
Copy Markdown
Contributor Author

@scott-huberty scott-huberty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporate @larsoner suggestions

scott-huberty and others added 2 commits September 24, 2025 10:23
Incorporate @larsoner suggestions

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@scott-huberty scott-huberty marked this pull request as ready for review September 24, 2025 19:58
@scott-huberty
Copy link
Copy Markdown
Contributor Author

Is this good to go other than the TODO in the code?

Yep I think so... Marked ready for review.

Copy link
Copy Markdown
Collaborator

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll push, update the version, and cut a quick release

@larsoner larsoner merged commit 9bd9889 into jackz314:main Sep 24, 2025
5 checks passed
@scott-huberty scott-huberty deleted the export_epochs branch September 24, 2025 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants