-
Notifications
You must be signed in to change notification settings - Fork 248
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
Fix ephys channel ids in spikegadgetsrawio #1303
base: master
Are you sure you want to change the base?
Conversation
I think setting the channel names to |
@samuelgarcia @JuliaSprenger could you please review this? would like this functionality on spikeinterface asap |
Hi @khl02007 Can you remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rly, just want to ping this to see if you want to figure out why tests are failing for when you have time :)
Co-authored-by: Zach McKenzie <[email protected]>
Co-authored-by: Zach McKenzie <[email protected]>
@rly we also ran black on the whole code base and did some cleanup with docstrings and asserts which have led to merge conflicts. If you have the bandwidth to fix those really quick that would be awesome! |
This is the error causing tests to fail. |
@zm711 it seems the test is failing to read analog signals because what is passed to
|
@khl02007 , thanks for checking. The way the testing works is it auto-detects what the possible streams are based on the reader and then iterates through them. So that means that the way the reader is implemented in this PR is not quite following the appropriate strategy for a Neo reader (ie this would need to be fixed at the Neo reader level). If this is the case it seems like you would need to add some more logic in the reader to prevent the reader itself from making mistakes between analog and digital signaling. Does that make sense? |
@zm711 I think I understand, but what exactly needs to be done to address this concern then? How does the reader automatically detect the possible streams? What is the appropriate strategy for a neo reader? |
I just talked to @samuelgarcia and he said he worked don this io before. He might be best to help a bit. He said he was having some struggles trying to get the additional streams due to the way the data is set up so it might be a bit more work to get this to work within the Neo structure. I would either reach out to him directly or ping him every once in a while. |
We discussed this a bit and from what Sam remembers this format would be very hard to fit within the model of So more concretely previously for the intanrawio to get access for the digital information you would have to do: reader = IntanRawIO()
reader.parse_header()
digital_data = reader._raw_data['DIGITAL-IN'] So you could do something similar where you store these channels in the raw data container, but you don't add them to channel info for now. The future goal is to have a separate digitalsignal stream that would allow us more flexibility in loading these types of signals, but that is on the backburner for now. |
def _produce_ephys_channel_ids(self, n_total_channels, n_channels_per_chip): | ||
"""Compute the channel ID labels | ||
The ephys channels in the .rec file are stored in the following order: | ||
hwChan ID of channel 0 of first chip, hwChan ID of channel 0 of second chip, ..., hwChan ID of channel 0 of Nth chip, | ||
hwChan ID of channel 1 of first chip, hwChan ID of channel 1 of second chip, ..., hwChan ID of channel 1 of Nth chip, | ||
... | ||
So if there are 32 channels per chip and 128 channels (4 chips), then the channel IDs are: | ||
0, 32, 64, 96, 1, 33, 65, 97, ..., 128 | ||
See also: https://github.com/NeuralEnsemble/python-neo/issues/1215 | ||
""" | ||
x = [] | ||
for k in range(n_channels_per_chip): | ||
x.append( | ||
[ | ||
k + i * n_channels_per_chip | ||
for i in range(int(n_total_channels / n_channels_per_chip)) | ||
] | ||
) | ||
return [item for sublist in x for item in sublist] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now in main. Let us know if you want to keep this open and try to get the digital channels working with Neo or whether you want to close this PR now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to @rly, but I think it may be best to just keep this open and work towards getting the analog and digital IOs loadable with Neo.
Fix #1215. Using code from @khl02007 - thanks!
This needs some more testing, but I am proposing this PR to continue the conversation.