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

Open Ephys: read events from all streams #1430

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

alejoe91
Copy link
Contributor

@alejoe91 alejoe91 commented Mar 8, 2024

Fixes #1367

@johnmbarrett could you test it on your side?

@alejoe91 alejoe91 added this to the 0.13.1 milestone Mar 8, 2024
# the node name to make it unique
oe_stream_name = info["folder_name"].split("/")[0] # remove trailing slash
if len(node_name) > 0:
stream_name = node_name + "#" + oe_stream_name
Copy link
Contributor

Choose a reason for hiding this comment

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

It's always a #? I have a feeling something is going to change in the future (because why would people be consistent :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is purely a naming on the NEO side ;) it's always been with the #

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool cool. Sounds good. :)

@johnmbarrett
Copy link

Seems to work insofar as I can load event times from all streams of my test file. There are a bunch of other problems with the durations and timestamps, but I think they're unrelated to this so I'll open new issues for those.

@johnmbarrett
Copy link

One thing I will note is that the Neuropixels AP and LFP event channels end up with the same name and ID in the resulting header structure inside the OpenEphysBinaryRawIO object, which means that one of them will be invisible to downstream packages (in particular spikeinterface) that access event channels by ID rather than a numeric index. Should that be addressed here or should I raise an issue with spikeinterface (once this change is incorporated into a neo release)? It's not catastrophic for my purposes since they're the same events and should have the same timestamps after accounting for sampling rate, but just thought I'd point it out.

@johnmbarrett
Copy link

Test dataset for the behaviour described in my last comment is available here: https://drive.google.com/file/d/1Hr3-GFOpOkNc1hgXdVr5HTunchQm4enw/view?usp=drive_link

@alejoe91
Copy link
Contributor Author

One thing I will note is that the Neuropixels AP and LFP event channels end up with the same name and ID in the resulting header structure inside the OpenEphysBinaryRawIO object, which means that one of them will be invisible to downstream packages (in particular spikeinterface) that access event channels by ID rather than a numeric index. Should that be addressed here or should I raise an issue with spikeinterface (once this change is incorporated into a neo release)? It's not catastrophic for my purposes since they're the same events and should have the same timestamps after accounting for sampling rate, but just thought I'd point it out.

How would you change the implementation to account for this? Maybe we can merge this one and you can make a PR to fix the other issue?

@johnmbarrett
Copy link

johnmbarrett commented Mar 26, 2024

How would you change the implementation to account for this? Maybe we can merge this one and you can make a PR to fix the other issue?

Just looking through the code, when the signal_streams list is set up each channel seems to be given a unique ID, but when setting up the equivalent event_streams list the channel name is just passed in twice (on line 187 for openephysbinaryrawio.py), once as the name and once as the ID. Seems like you could use stream name as the ID rather than the channel name (assuming one channel per stream for events, which I think is true since different TTL lines get assigned different labels in the event stream rather than different channels), or make up some identifier that's unique by construction. Or leave as is and document somewhere that event channels are not guaranteed to have unique IDs, then it's an issue for downstream packages.

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.

At least from my side this looks good and stops the clobbering. I think improvements could be a separate PR, no?

@alejoe91 alejoe91 merged commit 51f0246 into NeuralEnsemble:master Apr 5, 2024
1 check passed
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.

Can't read TTL events from multiple streams in an Open Ephys file
4 participants