Skip to content
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

ENH: Add BDF support #63

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

larsoner
Copy link
Contributor

Couldn't resist the temptation to try, didn't end up being too bad! Data reading now at least seems to work in MNE-Python and I "just" have some work to do to get the annotations working properly for our tests at least. Then hopefully it's not too much work to fix the rest here.

One tricky part will be supporting memmaps, but I'd be inclined to simply not support them for BDF data for now. Someone else could always add it later!

  • Fix annotation support
  • Disallow memmaps for BDF
  • Add tests

Closes #62

@hofaflo
Copy link
Contributor

hofaflo commented Jan 30, 2025

Thank you for working on this! :)

Something to keep in mind, but ignore for this PR: For unambiguous type annotations, default values, and possible future support of a "Status" channel, separate Bdf and BdfSignal classes would be helpful. We still plan on splitting up the EdfSignal class as described in #34, which should simplify doing this, but I can't give an estimate on when that will happen. Once there are separate classes for EDF and BDF, adding a read_bdf() would probably also make sense. This will also make the newly added parameter fmt redundant, but we can phase it out with deprecation warnings.

@larsoner
Copy link
Contributor Author

larsoner commented Jan 30, 2025

Something to keep in mind, but ignore for this PR: For unambiguous type annotations, default values, and possible future support of a "Status" channel, separate Bdf and BdfSignal classes would be helpful.

Yeah that could make sense! It actually would be very EDIT: hopefully fairly trivial for me to split the current PR into a Edf / Bdf that both inherited from BaseEdf that contained 99% of the current Edf class code. Let me know if that's preferable to do right now, it would probably simplify a lot of the logic here, too, and I wouldn't expect the diff to be huge since most of the existing Edf and EdfSignal code wouldn't move.

@hofaflo
Copy link
Contributor

hofaflo commented Jan 30, 2025

Yes, if you're willing to do that it'd be great! :)

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.

ENH: Interest in supporting BDF?
2 participants