-
Notifications
You must be signed in to change notification settings - Fork 261
[Neuralynx] Add multiple streams #1041
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
[Neuralynx] Add multiple streams #1041
Conversation
Hello @JuliaSprenger! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-05-13 11:17:03 UTC |
I tried this out and I get:
|
Hi @TheChymera, sorry for not making this clear in the first place: I opened this PR as a Draft as it is not complete yet. I am still working on the issue and will finalize the PR this week. |
@JuliaSprenger no worries, I just wanted to share feedback in case it might useful :) Also, what example data do you work with, did you get the full |
c8ced49
to
9412c97
Compare
@samuelgarcia I resolved conflicts with the current master and tests should pass now. Can you review if you have time? |
Hi Julia, Do we have somewhere something that check that the same stream have the same gaps ? |
Hi @samuelgarcia thanks for the feedback. You are right, the gap handling should happen first on a per-stream basis and then across streams as the requirements are a bit different. I will update the PR soon. |
Hi @JuliaSprenger , we are hacking in DANDI/NWB hackathon and @TheChymera apparently still uses his own, more hacky version. It would be great to see this PR to finalize/gets merged so we could use it instead. Please let us know if there is anything specific we could be of help with if anything. |
Failing IO tests are due to a certificate update on the gin side and outdated cached versions. Hopefully this issue will resolve itself in the next hours, see https://gin.g-node.org/G-Node/Info/issues/59 |
@samuelgarcia Thanks for having a first look. I addressed your comments. Can you have a look again? I am only waiting for confirmation to put some corresponding test files on gin to add a minimal stream test. |
TODO: Ensure consistent ordering of streams - done |
ba91fa2
to
eae66a3
Compare
@samuelgarcia I rebased on master and ordered stream by descending sampling rate. |
Perfect. Can I merge now ? |
If the tests still pass: Yes please merge it :) |
Salut Julia, |
...and capable of handling only a single package
Co-authored-by: Garcia Samuel <sam.garcia.die@gmail.com>
20373e1
to
bd33c15
Compare
@samuelgarcia I rebased on master and tests are passing now. Ready to merge. |
@samuelgarcia ping? |
To load multiple ncs files with different sampling rates multiple signals streams are required on the RawIO level. Here, for each set of channels sharing (sampling rate, n_samples, t_start) encountered in the files a dedicated signal stream is created to permit loading the data. Stream are ordered by descending sampling rate.
This PR also changes the event, epoch and spikes loaded for a segment. Previously all time point data were loaded for all segments, extending beyond the continuous signal of that segment. Now only time point data in the corresponding time frame of a segment are considered. If ncs data are available these define the segment structure. To load all time point data in a single segment all ncs files can be excluded resulting in a single segment containing all event, epoch and spiking data.
This PR is building on top of #990..
For the original discussion, see #1038