Spikeglx: use probe features from ProbeTable to infer Neuropixels type#1842
Spikeglx: use probe features from ProbeTable to infer Neuropixels type#1842alejoe91 wants to merge 5 commits into
Conversation
I'll take it! |
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
Thanks @chrishalcrow for testing this and sorry for taking so long @alejoe91. Actually, I was not sure if it was ready so I delayed it, sorry for assuming wrong. I have some requests regarding the handling of warnings vs error but otherwise LGTM.
Metacommentary:
After #1839 and #1842 we have our own copy of the json of the probe library. This is fine as long as both releases stay in lockstep with billkarsh's ProbeTable, but the cadence is controlled by two independent actions and two independent release cycles. A user with neo==X and probeinterface==Y where X and Y bracket a ProbeTable update might see diferences depending on versios. Honestly, it would be been simpler to just use probeinterface here rather than python-neo carrying its own copy. I do understand the French reason you probably decided against it though.
| with open(neuropixels_probe_features_file, "r") as f: | ||
| probe_features = json.load(f) | ||
|
|
||
| if probe_part_number is not None: |
There was a problem hiding this comment.
The two outer if probe_part_number is not None: and if features is not None: wrappers can collapse if we mirror probeinterface's Phase3A remap and raise instead of warning. This also picks up the Phase3A case (imProbeOpt instead of imDatPrb_pn), which probeinterface handles here but #1842 currently drops to unitary gain.
| if probe_part_number is not None: | |
| probe_part_number = meta.get("imDatPrb_pn", None) | |
| if probe_part_number is None and meta.get("imProbeOpt") is not None: | |
| probe_part_number = "NP1010" # Phase3A remap, matches probeinterface | |
| if probe_part_number is None: | |
| raise ValueError("Could not determine probe part number from metadata.") | |
| features = probe_features["neuropixels_probes"].get(probe_part_number) | |
| if features is None: | |
| raise ValueError(f"Probe part number {probe_part_number} not found in ProbeTable.") |
This replaces the warn-and-unitary-gain fallbacks with hard failures. We were doing this on master already with NotImplementedError for unsupported probes, and silently scaled-wrong traces are harder to catch downstream.
| gain_factor = float(meta["imAiRangeMax"]) / max_int | ||
| channel_gains = gain_factor * per_channel_gain * 1e6 | ||
| else: | ||
| warn( |
There was a problem hiding this comment.
Same argument applies inside the gain branch: I think we should raise an error Raising matches master's behaviour which I think is better than letting wrong gains silently propagate.
| warn( | |
| else: | |
| raise NotImplementedError( | |
| f"Probe {probe_part_number} has datasheet {datasheet!r}, " | |
| f"which is not currently supported by the SpikeGLX gain calculation. \n" | |
| "Please open an issue at python-neo repo" | |
| ) |
There was a problem hiding this comment.
This will become easier with a logic like the one in:
4 participants
Following #1839, the SpikeGLX reader is updated to infer the 1.0/2.0 probe type from the
datasheetfield of the associated entry in thenoe/resources/neuropixels_probe_features.json.In case the datasheet is not 1.0/2.0, a warning is pronted and unitary gains are added.