-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
FIX, TST: Try to get test_export_epochs_eeeglab passing again #13428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX, TST: Try to get test_export_epochs_eeeglab passing again #13428
Conversation
for more information, see https://pre-commit.ci
If https://github.com/jackz314/eeglabio/blob/f496ba45b5db716360e05c24efd89339f7bd2434/eeglabio/epochs.py#L101 is Correct, then MATLAB stores sample number starting at 1 and not at 0
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..
|
OK After 3 years, 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. |
|
Okay: https://pypi.org/project/eeglabio/0.1.2/ Should be able to revert the special-branch stuff and continue |
Co-authored-by: Eric Larson <[email protected]>
This reverts commit 51cff19.
larsoner
left a comment
There was a problem hiding this 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
* 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) ...
This test has been marked as failing for a while now:
mne-python/mne/export/tests/test_export.py
Lines 539 to 559 in 936bfae
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_sampof a file and 2) the true number of trials before epochs get dropped. AFAICT, the direct path forward is to add a new parameterexport_kwargstoepochs.export, that gets threaded toeeglabio.epochs.export_set. Then we can pass afirst_sampandn_trialsargument, which I am proposing to add over at jackz314/eeglabio#18Reference issue (if any)
What does this implement/fix?
Additional information