Skip to content

Discussion: deprecate read_imro #351

@h-mayorquin

Description

@h-mayorquin

When discussing this pull request with @chrishalcrow, I’ve been thinking about the following:

Why do we expose read_imro directly?

The private function _read_imro_string is the one actually used by SpikeGLX and Open Ephys, while read_imro is just a wrapper around it that, for a reason that I can't remember, is exposed.

To make read_imro work, we need quite a bit of logic to infer the imDatPrb_pn (probe model number) from imDatPrb_type (probe type). This code is complex and since it was determined through trial and error when we did not have a general table, maybe not very reliable:

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:
if len(imro_table_header) == 3:
# In older versions of neuropixel arrays (phase 3A), imro tables were structured differently.
probe_serial_number, probe_option, num_contact = imro_table_header
imDatPrb_type = "Phase3a"
elif len(imro_table_header) == 2:
imDatPrb_type, num_contact = imro_table_header
else:
raise ValueError(f"read_imro error, the header has a strange length: {imro_table_header}")
imDatPrb_type = str(imDatPrb_type)
else:
if imDatPrb_pn not in probe_part_number_to_probe_type:
raise NotImplementedError(f"Probe part number {imDatPrb_pn} is not supported yet")
imDatPrb_type = probe_part_number_to_probe_type[imDatPrb_pn]
probe_description = npx_descriptions[imDatPrb_type]

This was necessary when our probe descriptions were based on the probe type. However, now that the official metadata table is organized around imDatPrb_pn (probe part number), this complexity is no longer needed. In other words, this logic doesn’t serve the key functionality anymore, which is reading neuropixel metadata.

Additionally, I’m not sure exposing the functionality to read IMRO tables on their own makes sense. These tables are embedded in the meta file, and trying to read them independently seems like a feature no one has asked for.

Proposed solution:

  1. Move the logic for inferring imDatPrb_pn from imDatPrb_type out of the core function (_read_imro_string) and into the wrapper (read_imro), keeping the complexity at the periphery.
  2. If there is no use for stand-alone reading of imro tables then deprecate read_imro and eventually remove it. This will allow us to eliminate code that is no longer needed now that we can rely on the metadata table.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions