Enhance probegroup API

[alejoe91]

@samuelgarcia @h-mayorquin

Some changes on top of #446 that start addressing the points raised here + in #447:

New / changed

• added set_global_contact_order: helper function to set private variable
• select_contacts(contact_ids=..., probe_ids=...) — identifier-based selection (either or both), as proposed in Slice Probe and ProbeGroup by stable identifiers, not positional integers SpikeInterface/probeinterface#447. Layered on top of get_slice.
• probe_id support — add_probe(probe, probe_id=...), a probe_ids list, and set_probe_ids(). Defaults to probe_1, probe_2, …
• get_slice now propagates metadata — probe annotations are mapped to the correct sliced probe (robust to skipped probes) and probe_ids are carried over. This fixes the "annotations should be handled, not left as a TODO" comment.
• Added __repr__, extended examples (ex_03, ex_05) and grew the test suite (now covers set_global_contact_order, select_contacts in all modes, and get_slice annotation/probe_id propagation).
• Fixed .tolist() and the set_global_device_channel_indices docstring rename.
• Extended docs (ex_03/05 to show new behavior - probe_ids, select_contacts, global_contact_order, etc)

Re SpikeInterface#447
select_contacts is the identifier-based API #447 asks for, so this is a first step in that direction. I kept get_slice as the internal mechanism rather than removing it, but this is not in the docs

[samuelgarcia]

@samuelgarcia @h-mayorquin

Some changes on top of #446 that start addressing the points raised here + in #447:

New / changed

• added set_global_contact_order: helper function to set private variable

Not sure this is a good stuff, this contact order is tricky and should be internal only for the corner cases.

• select_contacts(contact_ids=..., probe_ids=...) — identifier-based selection (either or both), as proposed in Slice Probe and ProbeGroup by stable identifiers, not positional integers SpikeInterface/probeinterface#447. Layered on top of get_slice.

We should maintain the order requested no ?
I think I would prefer the probe_index instead of the probe_id.
This is more consistent with the internalk representaion which is a list and not a dict.
Or we go for dict but this would be a major changein the format.

• probe_id support — add_probe(probe, probe_id=...), a probe_ids list, and set_probe_ids(). Defaults to probe_1, probe_2, …

• get_slice now propagates metadata — probe annotations are mapped to the correct sliced probe (robust to skipped probes) and probe_ids are carried over. This fixes the "annotations should be handled, not left as a TODO" comment.

Thank you, let me check in detail it was tricky than it appear.

[alejoe91]

Let's address this @samuelgarcia

[alejoe91]

in a follow up PR

[alejoe91]

@samuelgarcia done what we discussed:

• select_contacts now requires probe_ids with same length as contact_ids unless contact ids are unique across probes (examples updated)
• added new select_probes function

[h-mayorquin]

Thanks, this addresses most of the concerns. One point on index-versus-ids for the Probes.

I am sympathetic to @samuelgarcia: we seldom have good ids for Probes. The natural one is the serial number, often unavailable and too verbose to be human-readable, so at first glance an index is the nicer handle. The problem is what select_contacts and re-aggregation do to it.

Take three Probes:

contacts	a1 a2 a3	b1 b2 b3	c1 c2 c3
Probe	A	B	C
Index	0	1	2

Now select_contacts({a1, c2, c3}), so ProbeB drops out. Do we re-index, or leave a gap? Neither is clean.

Option 1, re-index to stay contiguous. After select_contacts({a1, c2, c3}):

contacts	a1	c2 c3
Probe	A	C
Index	0	1

ProbeC went 2 -> 1, so the index is not a stable identity. Now re-aggregate ProbeB:

contacts	a1	c2 c3	b1 b2 b3
Probe	A	C	B
Index	0	1	2

ProbeB lands at 2 though it started at 1, with nothing recording that index 2 is the old 1. Identity is scrambled.

Option 2, leave a gap to keep each number. After select_contacts({a1, c2, c3}):

contacts	a1	c2 c3
Probe	A	C
Index	0	2

This keeps identity but defeats the index: it stops being a contiguous 0..n-1 enumeration and acts as a sparse label. And re-aggregating ProbeB still forces a choice, refill the gap with 1 so the index order no longer matches the list order:

contacts	a1	c2 c3	b1 b2 b3
Probe	A	C	B
Index	0	2	1

or append 3 so ProbeB's number changes from 1, losing identity again:

contacts	a1	c2 c3	b1 b2 b3
Probe	A	C	B
Index	0	2	3

There is no clean choice because the index is asked to be two things at once: a position (contiguous, reflects the structure) and a name (stable across operations). select_contacts then re-aggregation forces those apart, and no convention lets the index do both. An id only has the name job, so it travels with the probe and the conflict disappears.

So reaching for the index because ids are not human-readable is a cure that costs more than the disease: the id ergonomics are mildly annoying, but the index brings a real ambiguity into exactly the operations that started this. Giving Probes ids keeps identity stable through both and is easier to reason about.

[alejoe91]

Thanks, this addresses most of the concerns. One point on index-versus-ids for the Probes.

I am sympathetic to @samuelgarcia: we seldom have good ids for Probes. The natural one is the serial number, often unavailable and too verbose to be human-readable, so at first glance an index is the nicer handle. The problem is what select_contacts and re-aggregation do to it.

Take three Probes:

contacts a1 a2 a3 b1 b2 b3 c1 c2 c3
Probe A B C
Index 0 1 2
Now select_contacts({a1, c2, c3}), so ProbeB drops out. Do we re-index, or leave a gap? Neither is clean.

Option 1, re-index to stay contiguous. After select_contacts({a1, c2, c3}):

contacts a1 c2 c3
Probe A C
Index 0 1
ProbeC went 2 -> 1, so the index is not a stable identity. Now re-aggregate ProbeB:

contacts a1 c2 c3 b1 b2 b3
Probe A C B
Index 0 1 2
ProbeB lands at 2 though it started at 1, with nothing recording that index 2 is the old 1. Identity is scrambled.

Option 2, leave a gap to keep each number. After select_contacts({a1, c2, c3}):

contacts a1 c2 c3
Probe A C
Index 0 2
This keeps identity but defeats the index: it stops being a contiguous 0..n-1 enumeration and acts as a sparse label. And re-aggregating ProbeB still forces a choice, refill the gap with 1 so the index order no longer matches the list order:

contacts a1 c2 c3 b1 b2 b3
Probe A C B
Index 0 2 1
or append 3 so ProbeB's number changes from 1, losing identity again:

contacts a1 c2 c3 b1 b2 b3
Probe A C B
Index 0 2 3
There is no clean choice because the index is asked to be two things at once: a position (contiguous, reflects the structure) and a name (stable across operations). select_contacts then re-aggregation forces those apart, and no convention lets the index do both. An id only has the name job, so it travels with the probe and the conflict disappears.

So reaching for the index because ids are not human-readable is a cure that costs more than the disease: the id ergonomics are mildly annoying, but the index brings a real ambiguity into exactly the operations that started this. Giving Probes ids keeps identity stable through both and is easier to reason about.

So the probe_index is now mainly internal and it's always reset to be from 0 to N_probes - 1, in order of probe appearance. This PR also adds the probe_id to the numpy dtype, which makes propagation of annotations and contact selection unambiguous. I think though that we could get rid of probe_index completely, but @samuelgarcia was against it.

[h-mayorquin]

Should this be private?

[alejoe91]

yep, done in last commit

[h-mayorquin]

Suggested change

	probe_id = f"{len(self._probes)}"
	existing_int_ids = [int(pid) for pid in self._probe_ids if pid.isdigit()]
	probe_id = str(max(existing_int_ids, default=-1) + 1)

The default probe id comes from the probe count (f"{len(self._probes)}"), which assumes ids stay contiguous. Once a slice leaves a gap the count points back at an id that still exists.

Now (current, len):

start:                      ["0", "1", "2"]
select_probes(["0", "2"]):  ["0", "2"]        ("1" dropped, "2" kept)
add_probe(p):               len = 2  ->  "2"  ->  collides with existing "2", raises

With the proposal (max + 1):

start:                      ["0", "1", "2"]
select_probes(["0", "2"]):  ["0", "2"]        ("1" dropped, "2" kept)
add_probe(p):               max(0, 2) + 1 = "3"  ->  ["0", "2", "3"]

Non-numeric probe ids, with the proposal:

start:                      ["left", "right"]
add_probe(p):               no numeric ids, max(default=-1) + 1 = "0"  ->  ["left", "right", "0"]

max(existing numeric ids) + 1 is gap-proof, never recycles a removed probe's id, and the generated digit-string can never collide with a non-numeric name.

[alejoe91]

done in last commit

[alejoe91]

and added a test!

[h-mayorquin]

LGTM after all the changes, I added a bunch of suggestions. Feel free to ignore, I can take them later as a separate PR if you guys want expediency.

[alejoe91]

@h-mayorquin addressed all :) Note that the setter is gone, so this was not relevant: #1 (comment)