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

Fix ephys channel ids in spikegadgetsrawio #1303

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rly
Copy link

@rly rly commented Jul 6, 2023

Fix #1215. Using code from @khl02007 - thanks!

This needs some more testing, but I am proposing this PR to continue the conversation.

@JuliaSprenger JuliaSprenger added this to the future milestone Jul 19, 2023
@khl02007
Copy link
Contributor

I think setting the channel names to HWChan N (where N is channel index) would be clearer.

@khl02007
Copy link
Contributor

@samuelgarcia @JuliaSprenger could you please review this? would like this functionality on spikeinterface asap

@rly rly marked this pull request as ready for review December 14, 2023 22:50
@alejoe91
Copy link
Contributor

Hi @khl02007

Can you remove [WIP] from the PR name and see why tests are failing for spikegadgets?

@rly rly changed the title [WIP] Fix ephys channel ids in spikegadgetsrawio Fix ephys channel ids in spikegadgetsrawio Dec 15, 2023
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.

@rly, just want to ping this to see if you want to figure out why tests are failing for when you have time :)

neo/rawio/spikegadgetsrawio.py Outdated Show resolved Hide resolved
neo/rawio/spikegadgetsrawio.py Outdated Show resolved Hide resolved
rly and others added 2 commits May 8, 2024 13:21
@zm711
Copy link
Contributor

zm711 commented May 8, 2024

@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!

@zm711 zm711 modified the milestones: future, 0.14.0 May 9, 2024
@zm711
Copy link
Contributor

zm711 commented May 9, 2024

raw_unit16 = raw_unit8_mask.reshape(-1).view("int16").reshape(shape)
E ValueError: cannot reshape array of size 512 into shape (1024,0)

This is the error causing tests to fail.

@khl02007
Copy link
Contributor

@zm711 it seems the test is failing to read analog signals because what is passed to _get_analogsignal_chunk is block_index = 0, seg_index = 0, i_start = 0, i_stop = 1024, stream_index = 0 but stream_index=0 corresponds to Controller_DIO_digital. To get analog signal (ECU_analog) you have to pass stream_index=2. I would suggest changing the input in the test.

stream_index to stream_id mapping:
0 -> Controller_DIO_digital
1 -> ECU_digital
2 -> ECU_analog
3 -> trodes

@zm711
Copy link
Contributor

zm711 commented May 29, 2024

@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?

@khl02007
Copy link
Contributor

@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?

@zm711
Copy link
Contributor

zm711 commented May 30, 2024

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.

@zm711
Copy link
Contributor

zm711 commented Jun 7, 2024

@rly and @khl02007.

We discussed this a bit and from what Sam remembers this format would be very hard to fit within the model of analogsignal chunk that Neo uses because of the way that digital channels are stored. The hack that we used for another IO was to extract those values, but not to expose the stream at the Neo machinery level. Then for users that need that info they could just grab the private stream information from a private attribute.

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.

Comment on lines +89 to +108
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]

Copy link
Contributor

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?

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 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.

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.

Incorrect channel IDs in SpikeGadgetsRecordingExtractor
6 participants