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

[h-mayorquin]

dead code I think.

[h-mayorquin]

I think this should be handled in _extra_metadata_from_dict instead of here. This will avoid the polymorphism of the type in the signature and it will corral the serialization machinery to the serialization path and simplify the function that people are more likely to see.

[h-mayorquin]

There is also a strange behavior here when you pass a dict with with in_place=False which would be solve by this.

[h-mayorquin]

I am OK with all of this but there are a couple of features here:

1. Attached ProbeGroup instead of contact_vector.
2. set_probe/set_probegroup become in-place-only, plus the new select_channels_with_probe/select_channels_with_probegroup (effectively Move towards making set_probe in place by default #2798, breaking).
3. Geometry only through the probe instead of the location probe

The three look good to me but we can also make them as separate PRs if we don't feel OK about the whole bundle. Most of the diff churn comes from 2 I think.

Some comments:

• delete_annotation (no callers) and the average_across_direction float32 change look unrelated to the probe work and could be split out to make the diff simpler.
• The machinery for checking backwards compatibility looks OK but may be missing two cases. I would like to have more general infra for this as I mentioned in Move generate_ground_truth_recording to generation module #4588 (Move generate_ground_truth_recording to generation module #4588 (comment)). Out of scope here but I am glad that we are converging on the idea that something like this is necessary.

[alejoe91]

I agree @h-mayorquin

Let me split this into 2 PRs:
1+3 (which is very small) and then 2 ;)

That will make it easier to review. We can also postpone 2, since it might need some more discussion