Skip to content

Refactor probegroup to be array-based#420

Open
alejoe91 wants to merge 2 commits intoSpikeInterface:mainfrom
alejoe91:probegroup_as_array
Open

Refactor probegroup to be array-based#420
alejoe91 wants to merge 2 commits intoSpikeInterface:mainfrom
alejoe91:probegroup_as_array

Conversation

@alejoe91
Copy link
Copy Markdown
Member

In #416 we extended the ProbeGroup with a get_slice method. However, the current probe-centric implementation makes it difficult to slice in device channel indices from different probes are interleaved.

This PR refactors the ProbeGroup to be based on a _self.contact_array:

  • when a new probe is added, its array presentation is added to the _self.contact_array and we store probe contours and annotations (contact_annotations are in the array)
  • the probes is not a property that reconstructs the probes on-the-fly
  • importantly, changing annotations and contact annotations for a probe in a probegroup is now disabled. Added an annotate_probe(probe_index, **kwargs) to annotate probes in the probe group

Related to SpikeInterface/spikeinterface#4465, since we want to store a probegroup object directly, but we need to sort it/silce it according to the device_channel_indices

from .probe import Probe


class ProbeGroup:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alejoe91 we should make a very clear doc here for the new behavior.
We could explain teh why (need of global conact order across probes)
And also metioning the previous behavior (list of probe) and what cannot be possible now (annotate after)

@samuelgarcia
Copy link
Copy Markdown
Member

There is still an issue with this PR we need to solve : the to_dict/from_dict() and json will NOT preserve the contact order... We need to find something for it!!!

Give me a chance to add more complex test for orderings the probe group and global channel indices.

I propose a switch of version to 0.4.

@h-mayorquin
Copy link
Copy Markdown
Collaborator

I am taking a look. I will publish a review later today

@h-mayorquin
Copy link
Copy Markdown
Collaborator

I think this PR is doing too big of a change and we don't need to go that far.

First, some context. Currently we use ProbeGroup as a list-based probe container, and a dense representation of it attached to recording objects in SpikeInterface (a structured array produced by ProbeGroup.to_numpy() and stored as contact_vector). This split has led to problems: metadata and provenance are hard to recover (SpikeInterface #3498), and the wiring and attachment story is confusing (SpikeInterface #4397). We are therefore moving away from contact_vector and keeping a real ProbeGroup object on the recording, as in SpikeInterface #4465. This gives us a single object-level serialization layer, less reconstruction glue, and avoids duplicating probe-related functionality at the array level.

Having ProbeGroup as the attached object, we still want an array with the probe properties in channel order for convenience. The question is where that dense representation should live. This PR answers it by making ProbeGroup itself the dense representation: _contact_array becomes the authoritative backend, which in effect moves contact_vector inside ProbeGroup.

The move away from a simple list-based container, however, has real costs. For the array-backend to work, probegroup.probes has to become effectively immutable, because the real data now lives in _contact_array. As a consequence, the PR extends the coupling between ProbeGroup and Probe with a _probe_group back-reference on Probe plus guards on Probe.annotate and Probe.annotate_contacts. This is a case of inappropriate intimacy that already existed in the codebase (the back-reference was used by _check_compatible pre-PR) and that this PR takes further. The fragility of this approach is already visible: the guards are incomplete. Other Probe mutators (set_contact_ids, set_shank_ids, set_device_channel_indices, set_planar_contour, move, rotate, to_3d) still operate on the reconstructed probe and silently drop their effect because they were not guarded. Going all the way in this direction brings problems of its own and deepens the coupling between ProbeGroup and Probe.

There is a middle way. SpikeInterface does not need the full probe state flattened into a channel-ordered array, only a subset of it. I suggest we add a private _contact_vector property on ProbeGroup that holds just what SpikeInterface needs in channel order: positions, probe_index, shank_ids, and contact_sides. This touches less code and recovers the channel-facing behavior contact_vector used to provide, without rewriting the internal representation of the whole class. ProbeGroup's public identity stays probe-centric and probegroup.probes stays a durable list of Probe objects.

This is a narrower change that delivers what #4465 makes possible without the costs described above. To make this concrete, here are PRs that implement the idea here #425 and in SpikeInterface #4531.

Concretely, this would:

  • Keep the scope of the dense representation proportional to the recording-facing need. Only the subset SpikeInterface actually queries in channel order, rather than the full semantics of ProbeGroup.
  • Keep the benefit of #4465 (a real ProbeGroup object attached to the recording instead of a serialized contact_vector) without requiring the full backend rewrite this PR proposes.
  • Avoid deepening the inappropriate intimacy shown above. probegroup.probes remains a durable list of Probe objects, mutations on contained probes land on stored state, and there is no need to extend the existing _probe_group back-reference into mutation guards and synchronization logic.
  • Preserve ProbeGroup as the authoritative mutable object. The _contact_vector handle is derived state: invalidated on probegroup mutations and rebuilt lazily on first access.

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.

3 participants