SpikeInterface / spikeinterface

A Python-based module for creating flexible and robust spike sorting pipelines.
https://spikeinterface.readthedocs.io
MIT License
521 stars 186 forks source link

`recording.set_probegroup()` fails because because channel count in both match #2038

Closed stothe2 closed 1 year ago

stothe2 commented 1 year ago

I'm wondering why the below is >= and not >? As when attaching a probe group to a recording object, shouldn't it be OK if the channel counts in both exactly match?

https://github.com/SpikeInterface/spikeinterface/blob/ff1b8cdb59f0febb21354cb14a3ea2fc7577b202/src/spikeinterface/core/baserecordingsnippets.py#L147

Sorry if I'm missing something very obvious.

zm711 commented 1 year ago

https://github.com/SpikeInterface/spikeinterface/blob/ff1b8cdb59f0febb21354cb14a3ea2fc7577b202/src/spikeinterface/core/baserecordingsnippets.py#L146-L154

@stothe2, I'm just going to include more of the code link so we can see where number_of_device_channel_indices was created. I think the variable naming may be part of the confusion.

The operation above is taking the max of the device_channel_indices, which should be n-1 for channel number (0-indexing for python). So if the num of channels is 64, then the max of device_channel_indices should be 63 (so strictly < total number of channels). We can't take the len(device_channel_indices) because there is a possibility that some channels are disconnected and ignored so the max is taken to get the number of channels.

But correct me if I'm wrong @alejoe91 / @samuelgarcia .

As an aside there is a note asking where this check came from so might be worth revisiting. I guess one possible bug here would be that if the max device_indices is disconnected then it may return an erroneously too low value and pass this check.

stothe2 commented 1 year ago

Thank you for the detailed reply!

That makes sense. But is it required for device_channel_indices to be 0-indexed (or for that matter, even in sequential order?)? I had created a probe with indices starting from 1, and thus ran into this issue. You're correct that len(device_channel_indices) wouldn't work, but how about len([index for index in device_channel_indices if index != -1])?

Edit: I think np.max() is just to ensure that this particular code line works in case device_channel_indices is an empty list.

stothe2 commented 1 year ago

Actually on reading the code further, it looks like -1 or invalid device_channel_indices are already accounted for in the below lines. So simply len(device_channel_indices) should work.

https://github.com/SpikeInterface/spikeinterface/blob/ff1b8cdb59f0febb21354cb14a3ea2fc7577b202/src/spikeinterface/core/baserecordingsnippets.py#L135-L140

alejoe91 commented 1 year ago

@zm711 you are perfectly right! Thee device_channel_indices needs to be 0-based since it is the index of the traces which each contact correspond to, and Python is 0-based ;)

It would be good to double check those lines given the note, so let's keep this open for the moment!

alejoe91 commented 1 year ago

@stothe2 it doesn't need to be on sequential order! It should reflect the channel mapping between your probes and the acquisition system. We are correctly working on improving the docs to better explain this important step

samuelgarcia commented 1 year ago

Hi. The reponse of Zach is correct.

device_channel_indices is always zero based in probeinterface/spikeinterface api.

This is the link between contact_id and channel_id but with knowing in advance the channel_ids list. This is very convinient to describe a probe + mapping (that depend on a hardware) wihtout the channels names that mainly depend on the acquisition sofware (openephyx vs blackrock for instance.

For instance we could have 72 channels with 2 probes of 32 and 8 channels that are extra input. The probeA will certanilly be 0-31 channel indices and probeB 32-63 and then 64-71 would be the no prbe recording part. But sometimes some headstage can mix channels in a very strange order and the 2 probes can be interleaved! This is why we need this flexible device_channel_indices on probeinterface side.

zm711 commented 1 year ago

Also including links to the current probeinterface docs that demonstrate some real and some 'toy cases' of the wiring while the docs are being updated.

https://probeinterface.readthedocs.io/en/main/examples/ex_11_automatic_wiring.html https://probeinterface.readthedocs.io/en/main/examples/ex_05_device_channel_indices.html#sphx-glr-examples-ex-05-device-channel-indices-py

stothe2 commented 1 year ago

Thank you all for the quick, detailed responses! It truly helped. I assumed device_channel_indices needn't be 0-based, and was thus running into issues.