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

Merged
merged 6 commits into from
Dec 6, 2024

Conversation

vigji
Copy link
Contributor

@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
Contributor 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
zm711
zm711 previously approved these changes Dec 3, 2024
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
Copy link
Contributor

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

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

See comment

@zm711 zm711 dismissed their stale review December 6, 2024 13:06

stale

@alejoe91 alejoe91 merged commit 4061fdf into NeuralEnsemble:master Dec 6, 2024
3 checks passed
@samuelgarcia
Copy link
Contributor

Hi,
I was totally off for a while.
I read this very very quickly.
I suspect that this np.where() can slow down a lot the parse_header for long recording.
Is it a general or a corner case ?
We need to keep in mind that the parse header is done multiple time when multiprocessing on spikeinterface side so we need to be very carfull for this.
Do we have benchmark for this ?

@vigji
Copy link
Contributor Author

vigji commented Jan 6, 2025

Not data but rule of thumbs benchmark:
assuming 10 channels sampling TTL pulses at 100 Hz for 5 hours I generate a random array of integers from 0 to 10, np.where() then takes approx. 15 ms on my laptop (MacBook Pro M1 2022). This matches my experience that I never see long times even for fast digital signals recorded for a couple of hours.

Does this make sense?

@samuelgarcia
Copy link
Contributor

then it is ok. thank you for the fast anwser

@zm711
Copy link
Contributor

zm711 commented Jan 17, 2025

@vigji do we have you in our author list? I'm prepping our documents for new release. Would you like me to add you to the list (and what is the institution you were at/or are at for your Neo contribution?)

@vigji
Copy link
Contributor Author

vigji commented Jan 17, 2025

I don't think so as it was my first contribution!
Yes I'd like to, my institute is [optional: Center for Neuroscience and Cognitive systems], Istituto Italiano di Tecnologia (IIT), Italy

@zm711
Copy link
Contributor

zm711 commented Jan 17, 2025

Perfect. I will add you before our next release :) Thanks for the contribution!

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.

4 participants