-
Notifications
You must be signed in to change notification settings - Fork 47
Neuropixel: Add mux information to catalogue probes #383
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
base: main
Are you sure you want to change the base?
Neuropixel: Add mux information to catalogue probes #383
Conversation
719bd0c to
aa3e2aa
Compare
for more information, see https://pre-commit.ci
|
Hi Ramon I am not sure to get picture. For me, the mux table is something dynamic when selecting the 384 channels. Here the library is a "virtual full" neuroxpixels with 1024 channels so is it relevant to make this adc group and mux channel here ? |
The mapping from contact to multiplexing group is on the table/probe catalogue: And is fixed per probe type. At the moment, we don't add this information when building a probe from the The second contribution of the PR is conceptual. Separate the information of the catalogue from the dynamic selection at recording time encoded in the meta source file. |
| lf_sample_frequency_hz=float(probe_spec_dict["lf_sample_frequency_hz"]), | ||
| ) | ||
|
|
||
| # ===== 8. Add MUX table annotations ===== |
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.
@samuelgarcia
Here.
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.
@h-mayorquin I agree with @samuelgarcia here
The mux table is only defined for the selectable electrodes. E.g., for NP1.0, the adc_indices/groups will be 384, but the probe will have 900+ electrode.
So I think that setting contact_annotations will fail.
What we could do instead, is add the mux_table_string as annotation, since that applies to the probe model.
What do you 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.
Sorry @samuelgarcia. I was thinking that you were saying that maybe it did not make sense to add this information above but what you were hinting at is that the annotation of the mux information is wrong. And you are right. I went and assumed that the mux table was about contacts but is actually about readout channels so this information is not available without the meta. Thanks for pointing it out.
I have changed the PR to add the mux_table as an annotation.
I also added a test that fails with my previous logic so avoid both regressions and confusions like mine in the future.
|
Hi. @chrishalcrow do you want to check this ? |
| num_shanks = int(probe_spec_dict["num_shanks"]) | ||
| contacts_per_shank = int(probe_spec_dict["cols_per_shank"]) * int(probe_spec_dict["rows_per_shank"]) | ||
|
|
||
| if num_shanks == 1: |
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.
Hello, there's a problem here on the openephys side. For openephys (when there are no selected channels, which is the usual case), we compute the elec_ids using the contact position:
| if x_pitch == 0: |
Which gives a different result from this.
Not sure if it's an intractable difference between openephys and spikeglx?
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 problem here is that using the electrode positions to infer the electrode id might not be consistent with the reverse operation that we are doing to build the probe. That is:
forward-calculation: electrode_ids -> positions
backward-cacluation: positions -> electrodes_ids
might not be the same. Do you already have a case where things are inconsistent, @chrishalcrow?
Two important points for context:
- This problem is already on main
- I think that the new module of spikeglx is the one where
SELECTED_ELECTRODESis available.
Am I getting everything right, @chrishalcrow? If so, I think that this should not affect this PR and we can open an issue to see if this difference can be bridged.
Currently, the
get_probe()function from the library does not include MUX information for Neuropixels probes, while the format readers for spikeglx and openephys do. I started working on adding MUX information to the probes obtained withget_probewhen I noticed that the existing code structure made this task more complicated than necessary. To add MUX support to library probes, one would need to either duplicate the construction logic or refactor how_read_imro_string()and recording readers (.e.gread_spikeglx) work together.This PR introduces what I think is cleaner workflow based on a new function called
build_neuropixels_probe(). This function constructs a complete Neuropixels probe from the probe catalogue directly and in full. The new functioncan be used as a stand alone inget_probe()and as the first step on recording readers such as SpikeGLX and OpenEphys (before slicing and adding recording metadata not available in the catalogue). The advantage of this is that we have a single source of truth for Neuropixels probe construction, which simplifies maintenance and future updates.For the sake of the simplicity of the PR I am just changing the spikeglx workflow. Changing the openephys workflow and deprecating functions can be done in a follow up PR if this approach is accepted.