-
Notifications
You must be signed in to change notification settings - Fork 46
Changes needed to support ONIX #360
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?
Conversation
for more information, see https://pre-commit.ci
else: | ||
has_streams = False | ||
probe_names_used = 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.
The list of probe names for the test file are:
probe_names_used=['BreakoutBoard-DigitalIO', 'BreakoutBoard-AnalogIO-AnalogInput', 'BreakoutBoard-MemoryMonitor-PercentUsed', 'PortB-Neuropixels1.0eHeadstage-Probe', 'PortB-Neuropixels1.0eHeadstage-BNO055']
I think PortB-Neuropixels1.0eHeadstage-Probe
is the one we're interested int? What are the others??
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.
That is correct, the PortB-Neuropixels1.0eHeadstage-Probe
is the Neuropixels probe. The other streams are for other devices not related to Neuropixels. For reference, streams that start with BreakoutBoard
are devices on the ONIX Breakout Board, and can be ignored for the purposes of SpikeInterface parsing. The BNO055
is a device on the Neuropixels headstage, and can also be ignored in this PR.
custom_parameters = editor.find("CUSTOM_PARAMETERS") | ||
np_probes = custom_parameters.findall("NP_PROBE") | ||
if onix_processor is not None: | ||
np_probes = custom_parameters.findall("NEUROPIXELSV1E") |
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.
Will NEUROPIXELSV1E
always be the name of the 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.
I think it might be V1 or V2 depeneding on Neuropixels1.0/2.0. @bparks13 could you confirm?
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.
Currently the available options are NEUROPIXELSV1E
, NEUROPIXELSV1F
, and NEUROPIXELSV2E
. The V1E
/V1F
are both Neuropixels 1.0 probes, and can be used interchangeably at this stage, the E/F distinction is differentiating the other ONIX hardware/headstage.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #360 +/- ##
==========================================
+ Coverage 89.92% 90.11% +0.19%
==========================================
Files 12 12
Lines 2044 2084 +40
==========================================
+ Hits 1838 1878 +40
Misses 206 206 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
if onix_processor is not None: | ||
slot, port, dock = None, None, None | ||
probe_part_number, probe_serial_number = np_probe.attrib["probePartNumber"], np_probe.attrib["probeSerialNumber"] | ||
channels = np_probe.find("SELECTED_CHANNELS") |
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.
Here, the names of several settings are slightly different than the other cases. e.g. ONIX uses probePartNumber
instead of probe_part_number
.
Also, I don't think ONIX stores the slot, port and dock? Or maybe PortB
in the probe_names_used
says which port is used?
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.
In terms of naming, some of these elements have been introduced in the draft PR here, which gives us some flexibility with changing the names. I could modify the name to be probe_part_number
if that makes more sense to be consistent.
Since we are recording from Open Ephys hardware, and not IMEC hardware, we do not have a slot/port/dock to report. The Port in this instance is specifying which port on the Breakout Board the headstage is connected to.
xpos = np.array([float(electrode_xpos.attrib[ch]) for ch in channel_names]) | ||
ypos = np.array([float(electrode_ypos.attrib[ch]) for ch in channel_names]) | ||
positions = np.array([xpos, ypos]).T | ||
else: |
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 seems like the hardest thing: electrode positions are not saved with ONIX, but they are for the other formats.
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.
yep, I think that the easiest would be to store the electrode x/y positions, or the selected electrods. In the latter case, we could instantiate a full NP probe from the probe part number and slice it accordingly.
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.
We are currently storing the selected electrodes in the settings.xml file, under the SELECTED_CHANNELS
node. The indices are the global electrode index. I would be happy to talk through how it is currently set up, and we can determine if that information is enough or I can add in more details as needed.
@chrishalcrow @alejoe91 I have added some additional zipped folders containing some more diverse electrode configurations in the plugin PR: NPX 1.0 configurations NPX 2.0 configurations As per our discussion, you should not need the JSON files that are in the zipped folders, but the settings.xml files contain the selected electrode index and the probe part number, which should be sufficient. The JSON files will still be kept there for ease of use for the user, but are not needed in this PR. Let me know if you need anything else from me! |
Hey @bparks13 - thanks for all the new data. So when you have one probe the structure is e.g.
and the probe information is stored in the And when you have two or more it's e.g.
and the probe information is stored in the Is that right? Are the names |
@chrishalcrow The names are related to the NP version, as you mentioned. The difference in the structure is related to the physical hardware we use. Some headstages (NeuropixelsV1e/NeuropixelsV1f) have one probe per device, while NeuropixelsV2e headstages have one device with two probes:
I hope this helps! Let me know if anything doesn't make sense. I'm happy to meet and discuss it in more detail as well. |
Amazing - very helpful! |
This PR shows what changes would be needed to implement ONIX support directly using
read_openehys
. This would avoid having to make a separate.json
file and you get mux tables etc for free! It's a messy PR, just to see what the pain points are. I've written comments where I'm confused/unsure about something.Test data: open-ephys-plugins/onix-source#144 (comment)