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

OpenEphysBinaryRawIO: Fixing ttl multichan #1603

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vigji
Copy link

@vigji vigji commented Dec 2, 2024

Addressing #1437.

Currently, all the logic in the events parser for OpenEphys data assumes that there is a single stream of events (simply positive or negative states), and it breaks down when acquiring multiple digital signals at once (states having different absolute values for different channels).

This is a simple fix that looks for rising and falling edges separately for each of the digital channels.

Add simple testing relying on existing test data, which fails before the fix and passes afterwards.

@alejoe91
Copy link
Contributor

alejoe91 commented Dec 2, 2024

Thanks @vigji! Looking great!

@zm711 zm711 changed the title Fixing ttl multichan OpenEphyBinaryRawIO: Fixing ttl multichan Dec 2, 2024
@zm711 zm711 changed the title OpenEphyBinaryRawIO: Fixing ttl multichan OpenEphysBinaryRawIO: Fixing ttl multichan Dec 2, 2024
@vigji
Copy link
Author

vigji commented Dec 2, 2024

Great, let me know if there's anything needed before merging!

Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, although I would ask for one additional comment in the code to explain what version this works for (all or v0.6+ whatever) along with what the structure of the data is.

neo/rawio/openephysbinaryrawio.py Show resolved Hide resolved
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me, but Alessio knows this way better so I'll let him decide when to merge/do final review.

@zm711 zm711 added this to the 0.14.0 milestone Dec 3, 2024
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.

3 participants