Use ProbeGroup object instead of contact_vector property and set_probe/probegroup from select_channels_with_probe/probegroup#4465
Conversation
Co-authored-by: Alessio Buccino <alejoe9187@gmail.com>
| ) | ||
| # check that multiple probes are non-overlapping | ||
| all_probes = recording.get_probegroup().probes | ||
| check_probe_do_not_overlap(all_probes) |
There was a problem hiding this comment.
This is removed because when we split_by and aggregate probes might overlap
There was a problem hiding this comment.
I think this was important.
At least lets make a flag to control this.
There was a problem hiding this comment.
this needs to be removed. The issue is that before the aggregation was creating a single new probe, now it keeps a probegroup
|
I would like to take a look to this. I can do it this week. |
| ) | ||
| raise ValueError(error_msg) | ||
|
|
||
| new_channel_ids = self.get_channel_ids()[device_channel_indices] | ||
| probe_as_numpy_array = probe_as_numpy_array[order] | ||
| probe_as_numpy_array["device_channel_indices"] = np.arange(probe_as_numpy_array.size, dtype="int64") | ||
|
|
||
| # create recording : channel slice or clone or self | ||
| if in_place: | ||
| if not np.array_equal(new_channel_ids, self.get_channel_ids()): | ||
| raise Exception("set_probe(inplace=True) must have all channel indices") | ||
| sub_recording = self | ||
| else: | ||
| if np.array_equal(new_channel_ids, self.get_channel_ids()): | ||
| sub_recording = self.clone() | ||
| if len(keep_indices) == 0: |
There was a problem hiding this comment.
I think we can help the user more with raiseing here instead of havin an empty rec/probe
alejoe91
changed the title
set_probe/probegroup from select_channels_with_probe/probegroupProbeGroup object instead of contact_vector property and set_probe/probegroup from select_channels_with_probe/probegroup
|
@samuelgarcia @h-mayorquin now this is ready and final |
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
LGTM
The backward-compat fixtures cover single-probe (sequential and shuffled) and a contiguous two-probe recording, but not an interleaved multi-probe one (two probes whose device_channel_indices alternate, e.g. [0, 2, 4, 6, 1, 3, 5, 7]). That case does work on this branch, I checked it reloads with the correct per-channel locations thanks to _global_contact_order, but nothing tests it, so a future change to the reconstruction / get_slice / _global_contact_order could silently scramble channel-to-location alignment on load without CI noticing.
I am covering that in #4636 though.
Added the interleaved probegroup case. We cna unify backward compatibility tests later following #4636 |
| check_probe_do_not_overlap(probegroup.probes) | ||
|
|
||
| probegroup_sorted = self._get_probegroup_based_on_device_channel_indices(probegroup) | ||
|
|
There was a problem hiding this comment.
we need a checker that the deevice_channel_indices is equal to np.arange() here.
Because nothing prevent to have somethign like this [0, 2, 5, 9]
5 participants
Updated 7/1/26
@h-mayorquin after discussing with @samuelgarcia, we decided that 1 PR is ok
After SpikeInterface/probeinterface#446, we can go all in on the use of
ProbeGroupinstead ofcontact_vector, since the use case with interleaved contacts after device channel indices slicing is handled gracefully by the new_global_contact_orderarray, stored in the probegroup dictionary.Now the
recordinghas a_probegroupattribute, which is propagated as metadata. Theprobegroup.get_slicetakes care of all metadata propagation.In this refactoring, the duplicated
locationproperty has also been dropped.Note that this implementation is alternative to what is proposed in #4553 and implemented in #4548 : here we keep a
ProbeGroupobject attached to the recording, after sorting it bydevice_channel_indices, instead of adding awiringproperty. You lose the original probe contact order, but we argue that the contacts in a probegroup do not really have a "default" order, but are rather a set of contacts that become ordered when wired (see #3498).Some additional points:
Tests for aggregation/slicing
Added more tests for aggregation/slicing behavior and metadata propagation
Fixes #4545 #4546 #4547 #4549 #4553
Tests for backward compatibility
Added a test for bakward compatibility, which saves a recording to binary/zarr/JSON/pickle with v0.102/3/4.* and reloads it with the current version to check that the probegroup is reloaded correctly.
This way we check that the new
handle_extractor_backward_compatibility()handles all cases!set_probe/probegroupversusselect_channels_with_probe/probegroupThis PR refactors the
set_probefunction to address several issues:set_probe/probegroup: now are only in place (the not in place is deprecated, but will be removed in 0.106.0): these functions now require that the connected contacts have the same length as the number of channels.select_channels_with_probe/probegroupto handle the slicing of recordings with probes.Fixes #2193