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

Unify and extend handling of Neuropixels SYNC channel #1327

Merged
merged 12 commits into from
Nov 17, 2023

Conversation

alejoe91
Copy link
Contributor

@alejoe91 alejoe91 commented Sep 7, 2023

Fixes #1321

  • Handles all options and combinations of SYNC channels in SpikeGLX and Open Ephys.
  • Memmaps are sliced in the _get_analogsignal_chunk function, to be prepared for memmap refactoring #1265

TODO

  • Add test files on GIN
    • SpikeGLX without SYNC
    • SpikeGLX with less than 384 channels
    • Open Ephys with SYNC
  • Extend tests

@alejoe91
Copy link
Contributor Author

@JuliaSprenger @samuelgarcia extended tests for SpikeGLX and Open Ephys using the newly added GIN files (https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/110)

Ready to review!

@alejoe91
Copy link
Contributor Author

@samuelgarcia can you take a look at this? This is quite important on our side!

@samuelgarcia
Copy link
Contributor

Merci.
I made on comments.
As a general feedback, variable name change make the PR very tedious to read and prevent focusing on the real change!

@pep8speaks
Copy link

pep8speaks commented Nov 3, 2023

Hello @alejoe91! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 156:25: E125 continuation line with same indent as next logical line
Line 533:68: E261 at least two spaces before inline comment
Line 538:100: E501 line too long (107 > 99 characters)
Line 566:68: E261 at least two spaces before inline comment

Line 394:13: E125 continuation line with same indent as next logical line

Line 35:100: E501 line too long (103 > 99 characters)
Line 44:100: E501 line too long (110 > 99 characters)
Line 54:100: E501 line too long (109 > 99 characters)
Line 66:100: E501 line too long (110 > 99 characters)
Line 70:100: E501 line too long (111 > 99 characters)
Line 80:100: E501 line too long (120 > 99 characters)

Comment last updated at 2023-11-16 14:19:34 UTC

@alejoe91
Copy link
Contributor Author

alejoe91 commented Nov 3, 2023

@samuelgarcia tried to fix failing Maxwell tests, but that doesn't work.

On the spikeinterface side, setting an ENV variable in the GH action seems to work: SpikeInterface/spikeinterface#2161

Should I push something similar here?

Nevertheless, it's unrelated to this PR

@samuelgarcia
Copy link
Contributor

Amazing work.

@samuelgarcia samuelgarcia merged commit 106eefe into NeuralEnsemble:master Nov 17, 2023
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.

Correct handling of Sync channel for SpikeGLX and Open Ephys
3 participants