-
-
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 <larson.eric.d@gmail.com>
This reverts commit 51cff19.
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
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_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 parameterexport_kwargs
toepochs.export
, that gets threaded toeeglabio.epochs.export_set
. Then we can pass afirst_samp
andn_trials
argument, which I am proposing to add over at jackz314/eeglabio#18Reference issue (if any)
What does this implement/fix?
Additional information