-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add BDF export #13435
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
base: main
Are you sure you want to change the base?
Add BDF export #13435
Conversation
for more information, see https://pre-commit.ci
Any ideas why the ultraslow_pg job is failing? The failing Windows pip pre job is due to pyvistaqt DeprecationWarnings, so this is unrelated I guess. The Ubuntu pip pre job segfaults, also due to some VTK issue. |
It looks like |
Sure, but if you want I can also refactor it. |
Sure feel free! On Azure this failure does look related:
but maybe the refactoring already took care of it. I'll tackle the unrelated errors in #13434 |
I don't understand this error. The new |
Found it and hopefully fixed it. |
Looks good, feel free to merge @larsoner! |
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.
LGTM, just a couple minor things
annotations.append(EdfAnnotation(onset, duration, desc)) | ||
|
||
Edf( | ||
annotations.extend(pad_annotations) |
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.
IIUC, doing it this way will change the order of the items in the annotations
list (previously, all the BAD_ACQ_SKIP` would be at the beginning, now they will be at the end). Does that matter? Or does EDF/BDF not care (or does the writer sort the annots in any case)?
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.
I thought it didn't really matter (the docs are not clear or I couldn't find that info). @hofaflo?
Now that edfio 0.4.10 added support for BDF, we should support that in MNE as well. The current implementation duplicates a lot of EDF code, which I think is OK for now (I'd prefer to refactor in a follow-up PR).