Use ProbeGroup object instead of contact_vector property

[alejoe91]

Updated

After SpikeInterface/probeinterface#446, we can go all in on the use of ProbeGroup instead of contact_vector, since the use case with interleaved contacts after device channel indices slicing is handled gracefully by the new _global_contact_order array, stored in the probegroup dictionary.
Now the recording has a _probegroup attribute, which is propagated as metadata.
Now real issue with bakward compatibility, since the saving to folder/zarr kept the device_channel_indices.
The probegroup.get_slice takes care of all metadata propagation.

Note that this implementation is alternative to what is proposed in #4553 and implemented in #4548 : here we keep a ProbeGroup object attached to the recording, after sorting it by device_channel_indices, instead of adding a wiring property. 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:

set_probe/probegroup versus select_channels_with_probe/probegroup

This PR further refactors the set_probe function 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.
• added select_channels_with_probe/probegroup to handle the slicing of recordings with probes.

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 with v0.104.* and reloads it with the current version to check that the probegroup is reloaded correctly.

[alejoe91]

This is removed because when we split_by and aggregate probes might overlap

[h-mayorquin]

I would like to take a look to this. I can do it this week.

[samuelgarcia]

I guess that @zm711 and @h-mayorquin are now dancing on their seat just reading this PR.

Let me read it slowly.

[h-mayorquin]

As we discussed previously I think this is a step in the direction direction.

Is _build_probegroup_from_properties only for backwwards compatbility?

I think we can segregate the backward-compatibility serialization logic (_build_probegroup_from_properties) from the main class methods by moving it into the deserialization hooks. _extra_metadata_from_dict already runs after properties are set (base.py:1202), so it can check for "probegroup" first and fall back to reconstructing from contact_vector. For the folder path, load_metadata_from_folder (base.py:639) needs to swap lines 643 and 646 so .npy properties load before _extra_metadata_from_folder runs, giving the hook access to legacy contact_vector when there is no probe.json. This has the side effect of a cleaner design where has_probe() becomes return self._probegroup is not None with no mutation at check time.

[alejoe91]

As we discussed previously I think this is a step in the direction direction.

Is _build_probegroup_from_properties only for backwwards compatbility?

I think we can segregate the backward-compatibility serialization logic (_build_probegroup_from_properties) from the main class methods by moving it into the deserialization hooks. _extra_metadata_from_dict already runs after properties are set (base.py:1202), so it can check for "probegroup" first and fall back to reconstructing from contact_vector. For the folder path, load_metadata_from_folder (base.py:639) needs to swap lines 643 and 646 so .npy properties load before _extra_metadata_from_folder runs, giving the hook access to legacy contact_vector when there is no probe.json. This has the side effect of a cleaner design where has_probe() becomes return self._probegroup is not None with no mutation at check time.
_extra_metadata_from_folder

Let's check also how zarr does it. If we don't have a separate function for zarr, we can also add it to __init__

[alejoe91]

this is not needed, handled by the save_to_folder

[alejoe91]

As we discussed previously I think this is a step in the direction direction.
Is _build_probegroup_from_properties only for backwwards compatbility?
I think we can segregate the backward-compatibility serialization logic (_build_probegroup_from_properties) from the main class methods by moving it into the deserialization hooks. _extra_metadata_from_dict already runs after properties are set (base.py:1202), so it can check for "probegroup" first and fall back to reconstructing from contact_vector. For the folder path, load_metadata_from_folder (base.py:639) needs to swap lines 643 and 646 so .npy properties load before _extra_metadata_from_folder runs, giving the hook access to legacy contact_vector when there is no probe.json. This has the side effect of a cleaner design where has_probe() becomes return self._probegroup is not None with no mutation at check time.
_extra_metadata_from_folder

Let's check also how zarr does it. If we don't have a separate function for zarr, we can also add it to __init__

@h-mayorquin actually both the save_to_folder and save_to_zarr save the probegroup already to json/attrs. So I think that removing the contact_vector entirely would still keep backcompatibility. I'll give it a try with a comment!

[alejoe91]

Depends on SpikeInterface/probeinterface#420

[alejoe91]

@h-mayorquin @samuelgarcia this is ready to review. See updated PR description