Skip to content

Conversation

@scott-huberty
Copy link
Contributor

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

This test has been marked as failing for a while now:

def test_export_epochs_eeglab(tmp_path, preload):
"""Test saving an Epochs instance to EEGLAB's set format."""
eeglabio = pytest.importorskip("eeglabio")
raw, events = _get_data()[:2]
raw.load_data()
epochs = Epochs(raw, events, preload=preload)
temp_fname = tmp_path / "test.set"
# TODO: eeglabio 0.2 warns about invalid events
if _compare_version(eeglabio.__version__, "==", "0.0.2-1"):
ctx = _record_warnings
else:
ctx = nullcontext
with ctx():
epochs.export(temp_fname)
epochs.drop_channels([ch for ch in ["epoc", "STI 014"] if ch in epochs.ch_names])
epochs_read = read_epochs_eeglab(temp_fname, verbose="error") # head radius
assert epochs.ch_names == epochs_read.ch_names
cart_coords = np.array([d["loc"][:3] for d in epochs.info["chs"]]) # just xyz
cart_coords_read = np.array([d["loc"][:3] for d in epochs_read.info["chs"]])
assert_allclose(cart_coords, cart_coords_read)
assert_array_equal(epochs.events[:, 0], epochs_read.events[:, 0]) # latency

Specifically right now the test on Line 559 is failing, i.e. the event latencies change across an IO trip.

The culprit is this block of code:

https://github.com/jackz314/eeglabio/blob/cf92efeb92f6ac45f28df50014b4f23ffe622d47/eeglabio/epochs.py#L104-L109

Basically, I think that eeglabio needs to be able to handle 1) the first_samp of a file and 2) the true number of trials before epochs get dropped. AFAICT, the direct path forward is to add a new parameter export_kwargs to epochs.export, that gets threaded to eeglabio.epochs.export_set. Then we can pass a first_samp and n_trials argument, which I am proposing to add over at jackz314/eeglabio#18

Reference issue (if any)

What does this implement/fix?

Additional information

@scott-huberty scott-huberty marked this pull request as draft September 23, 2025 15:42
@scott-huberty
Copy link
Contributor Author

OK After 3 years, test_export_epochs_eeglab is passing : )

https://github.com/mne-tools/mne-python/actions/runs/17961850120/job/51086635335?pr=13428#step:19:3897

I am going to leave this PR in draft mode as a safeguard because I change the dependencies to point eeglabio to my branch over at https://github.com/jackz314/eeglabio/pull/18/files, which this PR depends on. The bulk of the fix is happening over there.

@larsoner
Copy link
Member

Okay: https://pypi.org/project/eeglabio/0.1.2/

Should be able to revert the special-branch stuff and continue

Copy link
Member

@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.

Other than one reST issue, LGTM +1 for merge! Feel free to commit the fix, mark as ready for review, and mark for merge-when-green @scott-huberty

@scott-huberty scott-huberty marked this pull request as ready for review September 24, 2025 21:06
@larsoner larsoner enabled auto-merge (squash) September 24, 2025 21:43
@larsoner larsoner merged commit 3e29187 into mne-tools:main Sep 24, 2025
32 checks passed
@scott-huberty scott-huberty deleted the test_export_epocsh_eeeglab branch September 24, 2025 22:36
larsoner added a commit to myd7349/mne-python that referenced this pull request Oct 22, 2025
* upstream/main: (46 commits)
  MAINT: Restore edfio git install (mne-tools#13421)
  Support preload=False for the new EEGLAB single .set format (mne-tools#13096)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13453)
  MAINT: Restore PySide6 6.10.0 testing (mne-tools#13446)
  MAINT: Auth [skip azp] [skip actions]
  MAINT: Deploy [circle deploy] [skip azp] [skip actions]
  Bump github/codeql-action from 3 to 4 in the actions group (mne-tools#13442)
  ENH: Dont constrain fiducial clicks to mesh vertices (mne-tools#13445)
  Use timezone-aware ISO 8601 for website timestamp (mne-tools#13347)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13443)
  FIX: Update osf.io links to new format (mne-tools#13440)
  MAINT: Ensure full checkout is used (mne-tools#13439)
  Add BDF export (mne-tools#13435)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13434)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13431)
  MAINT: Update code credit (mne-tools#13432)
  FIX, TST: Try to get test_export_epochs_eeeglab passing again (mne-tools#13428)
  FIX: Add on_few_samples parameter to core rank estimation (mne-tools#13350)
  MAINT: Reenable mpl nightly (mne-tools#13426)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13427)
  ...
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