-
Notifications
You must be signed in to change notification settings - Fork 47
Remove legacy probe construction path for read_imro and support NP1110
#410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
alejoe91
merged 19 commits into
SpikeInterface:main
from
h-mayorquin:remove_legacy_path_1
Mar 20, 2026
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
47ac78b
Remove legacy probe construction path for `read_imro`
h-mayorquin f8af14a
Merge branch 'main' into remove_legacy_path_1
h-mayorquin 8ba7849
add support for edge cases
h-mayorquin 9e3fa8a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 355754f
done, comments and warning
h-mayorquin fd1c152
Merge remote-tracking branch 'refs/remotes/origin/remove_legacy_path_…
h-mayorquin adc7856
naming
h-mayorquin aae913c
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 589aaaa
add header to imro parsing
h-mayorquin 45dae67
merge
h-mayorquin cfeaa1f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 7201ffc
unify slicing
h-mayorquin d24c57e
Merge remote-tracking branch 'refs/remotes/origin/remove_legacy_path_…
h-mayorquin 041afc4
reorganize and remove unused functions, add test files and tests for …
alejoe91 8178f3e
Merge branch 'main' into remove_legacy_path_1
alejoe91 d5c25f1
Finalize tests
alejoe91 c12c326
move _build_canonical_electrode_id to utils section
alejoe91 9d182d0
solve conflicts
alejoe91 90a202f
Add tests agains snsGeomMap and remove warning
alejoe91 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
imro_np1000andimro_np1110don't haveelectrodeas a key (probeinterface/src/probeinterface/resources/neuropixels_probe_features.json
Line 2261 in 208a95b
which is what all bank-related logic below (deleted lines 613-621) refer to. So you need to keep that logic in. Annoyingly, we don't have test data for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bank-y logic should be added to
read_spikeglxtoo, I think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: SpikeInterface/spikeinterface#3913
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Neuropixels 1.0 I codified the fallback in a new function
_resolve_active_contacts_for_np1that encapsulates theelectrode = bank * 384 + channellogic. Bothread_spikeglxandread_imronow call_get_active_contact_idswhich resolves electrode IDs if missing and builds the canonical contact ID strings. This function dispatches to the appropriate resolver based on the IMRO fields present. The IMRO parsing itself stays in_parse_imro_string, which is now a pure parser that returns a dict of IMRO fields without any electrode resolution (the electrode computation that was previously inline there has been extracted into the resolver functions).For NP1110 I went and checked how it is done in the C++ implementation (
IMROTbl_T1110.cppin the SpikeGLX repo). I threw an agent at it and had it implement the same logic here, which is now in_resolve_active_contacts_for_np1110. I added enough caveats in the function (a warning, a TODO, a comment explaining why we're mirroring the C++ naming) until we get real test data, but I think it should be OK to move forward.I also inlined all the logic for building the probe into
read_imrodirectly so it is even more likeread_spikeglx, instead of relying on a private method that was doing most of the computation.