Document contact_id uniqueness and enforce it on (probe_index, contact_id) - #463
Open
adityasingh2400 wants to merge 1 commit into
Open
Document contact_id uniqueness and enforce it on (probe_index, contact_id)#463adityasingh2400 wants to merge 1 commit into
adityasingh2400 wants to merge 1 commit into
Conversation
…t_id) Probe.__init__ and set_contact_ids both promised that contact_ids are unique at Probe AND ProbeGroup level, but cross-probe enforcement was removed in SpikeInterface#229 and the docs were never updated. _check_global_device_wiring_and_ids validated only device_channel_indices despite its name covering ids. Per the design agreed in the issue, contact_ids stay unique within a Probe and the key that is unique across a ProbeGroup is the pair (probe_index, contact_id). Downloading the same probe model twice and putting both in a ProbeGroup keeps their contact_ids as they are, which is the expected behaviour. - _check_global_device_wiring_and_ids now validates the compound key, so the method matches its name. - The comment in Probe.__init__, the set_contact_ids docstring and the get_global_contact_ids docstring now describe the real semantics and point at probe_ids for disambiguating an id shared by several probes. - Both checks now share a single to_numpy call rather than one each. Fixes SpikeInterface#402
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #402.
Probe.__init__andset_contact_idsboth promised contact_ids are unique at Probe and ProbeGroup level, but cross-probe enforcement was removed in #229 and the docs were never updated._check_global_device_wiring_and_idsvalidated onlydevice_channel_indicesdespite its name covering ids.This follows the design agreed in the thread: contact_ids stay unique within a Probe, and the key unique across a ProbeGroup is the pair
(probe_index, contact_id). Downloading the same probe model twice and putting both in a ProbeGroup keeps their contact_ids as they are, which is the expected behaviour._check_global_device_wiring_and_idsnow validates the compound key, so the method matches its name. TheProbe.__init__comment, theset_contact_idsdocstring and theget_global_contact_idsdocstring now describe the real semantics and point atprobe_idsfor disambiguating an id shared by several probes. Both checks now share a singleto_numpycall rather than one each.Tests:
test_duplicate_contact_ids_across_probes_are_allowedandtest_generate_dummy_probe_group_repeats_contact_ids_across_probeslock in the agreed semantics, and would fail if group-wide uniqueness were restored.test_check_global_ids_rejects_within_probe_duplicatescovers the newly real check.Worth noting as the concrete case this has to permit: the dummy probe group has 64 contacts but only 32 unique contact_ids, with all 64 compound keys unique. That is now a test.
Reverting the source from the base ref makes the new check test fail with
DID NOT RAISE ValueError. The two semantics tests pass in both halves by design, since they are regression guards on existing behaviour. Full suite is green at 191 passed and 1 skipped.Disclosure: this change was prepared with AI assistance. I have reviewed and tested it.