-
Notifications
You must be signed in to change notification settings - Fork 47
Use ProbeTable to generate NP probes information #349
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
Use ProbeTable to generate NP probes information #349
Conversation
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 89.75% 89.91% +0.15%
==========================================
Files 12 12
Lines 1972 2012 +40
==========================================
+ Hits 1770 1809 +39
- Misses 202 203 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
|
Thanks for taking on this, @chrishalcrow
Yes, you will need to modify the pyproject.toml to ensure that this is packaged on the wheel/stds. An alternative is to store it as a python dict which will make package simpler (that way you don't need to modify the gitignore). All that said, json should be fine as well, any reason you prefer it? Whatever we choose, we should add a cron test that runs weekly or monthy to ensure that what we have matches: https://github.com/billkarsh/ProbeTable/blob/main/Tables/probe_features.json I can help with that if you want. |
|
|
||
| is_phase3a = False | ||
| # These are all prototype NP1.0 probes, not contained in ProbeTable | ||
| if probe_part_number in ["PRB_1_4_0480_1", "PRB_1_4_0480_1_C", "PRB_1_2_0480_2", None]: |
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.
PRB_1_4_0480_1 and PRB_1_4_0480_1_C are on the table, why are we fetching NP1010 instead for those?
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.
Oops - tests were failing for these and we used to group them together so I thought that was why. Now realise there's a typo in PRB_1_2_0480_2 in the ProbeTable. Will raise an issue over there.
I think this is a good idea.I am +1 on it. |
for more information, see https://pre-commit.ci
| imro_table_header_str, *imro_table_values_list, _ = imro_str.strip().split(")") | ||
| imro_table_header = tuple(map(int, imro_table_header_str[1:].split(","))) | ||
|
|
||
| if imDatPrb_pn is None: |
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.
Note: we push this logic to the read_imro function.
for more information, see https://pre-commit.ci
|
The logic seems fine and the tests are passing. We can improve later if we need to so this is fine to me. To points left are that:
|
|
Great! I've...
Not sure how to check the packaging stuff - we can discuss offline. |
|
Me and @h-mayorquin discussed the GitHub action and checked if the |
|
|
||
| # annotate with MUX table | ||
| if mux_table is not None: | ||
| print("Adding MUX table to probe") |
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.
| print("Adding MUX table to probe") |
|
|
||
| if probe_part_number not in ["3000", "1200"]: | ||
|
|
||
| probe_info = make_npx_description(probe_part_number) |
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.
Now that we've removed the make_npx_description function, it's pretty difficult to test backwards compatibility inside the library, so we'll just get rid of this test?
We could externally make a repo that builds the probes from two ProbeInterface versions and compares them.
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.
Could we copy the old make_npx_description here in this tests and keep it for a while ?
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.
finally no too complex
| probe = _make_npx_probe_from_description(pt_metadata, elec_ids, shank_ids, mux_table) | ||
|
|
||
| # this is scalar annotations | ||
| mux_table_format_type = pt_metadata["mux_table_format_type"] |
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.
Should delete these three lines, and the line mux_table_array=mux_table_array, below
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.
you do not want to annotate the mux ?
| @@ -0,0 +1,495 @@ | |||
| from probeinterface.neuropixels_tools import make_npx_description, probe_part_number_to_probe_type | |||
|
|
|||
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.
| # this code is here only for testing the refactoring of neuropixel probe handling done by Chris in 2025 and based on ..... | |
| # this test could be removed in 202X .... | |
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.
filenally remove this file
| @@ -0,0 +1,2086 @@ | |||
| { | |||
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 would rename this file to neuropixels_probe_features.json no ?
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.
done
load json only once on demand remove backwards compativility file because it is not possible anymore to have it.
load json only once on demand remove backwards compativility file because it is not possible anymore to have it.
for more information, see https://pre-commit.ci
…ce into use-mux-tables
|
This is OK for me for merging. @alejoe91 @chrishalcrow I guess that the mux array annotations will be used in get_neuropixels_sample_shifts on spikeinterface side. |
|
ET voilà! |
|
Hey guys, thanks for taking this to the finishing line and proving me wrong when I said this would take a while : ) |
PR to use ProbeTable (https://github.com/billkarsh/ProbeTable/) to generate NeuroPixels probe information, instead of hardcoding this ourselves.
Summary
Import differences in
src/probeinterface/neuropixels_tools.pyDoes a few things:
npx_descriptionshardcoded dict, with information we can read from ProbeTable.mux_table_arrayfrom theprobe_features.json, file, which tells us the multiplexing groupings. This can then be used in spikeinterface for theinter_sample_shift. PR incoming. This is saved as a probe annotation.probe_typefrom metadata creation, and from theread_openephysandread_spikeglxfunctions. Still needed for the read/write imro functions, whereprobe_typeis stored in the format. E.g.tests/data/imro/test_single_shak_2.0.imrostarts “(21,384)(…” and the “21” is theprobe_typeof the probe.get_probe_contour_verticesfunction which computes the probe contour vertices from metadata.We now additionally support the following probes:
'NP2005', 'NP3011', 'NP1033', 'NP1122', 'NP3010', 'NP1120', 'NP1210', 'NP1014', 'NP3021', 'NP2020', 'NP3020', 'NP1011', 'NP3000', 'NP1013', 'NP1123', 'NP2021', 'NP1200', 'NP2006', 'NP1012'We LOSE support for
NP1110Implementation questions and annoying edge cases:
model_namefor each probe, which ProbeTable doesn’t contain. @alejoe91 suggested using thedescription, which changes all our probe names. Ok??jsonfile atsrc/probeinterface/resources/probe_features.json. I think it has to be in source to work for a normal pip installation? Any better places to store?npx_description, so it's not too radical. So we only really need check that the old hardcoded dict matches the new function. To do this, I’ve moved the old hardcoded dict to a test file. Any other ideas for how to check this?Backwards compatibility
There are some mismatches between the ProbeTable and old ProbeInterface implementation we found by comparing the
npx_descriptionsdict with themake_npx_descriptionsfunction (which now hasn't survived during the evolution of the PR). These are:We also tested the
neuropixelsdirectory from theprobeinterface_libraryand the new library generated by runninggenerate_neuropixels_library.py. We checked their differences using the following code. First, we checks which probes were generated from each library:The
new_not_oldcontains all the new probes which are in ProbeTable but not in our previous table. Nice! Theold_not_newcontainedNP1110which we currently don't support (@alejoe91 @samuelgarcia I think we're not supporting it, right??)We can then check that all the generated probes are equal by comparing their
jsonfiles. Here's some code that does that. We do assertions, then I've addedcontinues when we want to skip a property (if we know it's different):The skipped properties exactly match those in the table above.