Skip to content

Derive instruments.{VICAR,FITS}_INSTRUMENTS from a single canonical list #13

@rfrenchseti

Description

@rfrenchseti

Background

src/picmaker/instruments/__init__.py declares three hand-curated lists:

VICAR_INSTRUMENTS = [cassini, galileo, voyager]
FITS_INSTRUMENTS = [hst, nh]
ALL_INSTRUMENTS = [cassini, voyager, galileo, hst, nh]

Adding a new instrument requires updating all three lists (or two, if it only handles one format). The lists are independently mutable, which makes it possible for them to drift apart — e.g., a new instrument added to ALL_INSTRUMENTS but forgotten in FITS_INSTRUMENTS would silently fail to detect FITS files for that instrument.

See CODEBASE_CRITIQUE.md §1 (Structure and layout, "Finding (Medium) — instruments/__init__.py ships three explicit instrument lists").

Proposed approach

Option A — probe-based derivation. Drop the explicit VICAR_INSTRUMENTS / FITS_INSTRUMENTS lists; iterate ALL_INSTRUMENTS and let detect_vicar / detect_fits short-circuit on instruments that don't handle that format (they already return None unconditionally). This is the minimal change and adds zero new API surface; the cost is calling a no-op detect_* for every probe (negligible — six instruments today).

# instruments/__init__.py
ALL_INSTRUMENTS = [cassini, voyager, galileo, hst, nh]
# VICAR_INSTRUMENTS / FITS_INSTRUMENTS removed.

# io.py reader cascade
for instrument in instruments.ALL_INSTRUMENTS:
    filter_info = instrument.detect_vicar(vic)  # returns None for non-VICAR
    if filter_info is not None:
        return (array3d, False, filter_info)

Option B — declarative flags on the instrument module. Each instrument module declares SUPPORTS_VICAR: bool and SUPPORTS_FITS: bool module-level constants; the package derives the filtered lists from those flags:

# instruments/cassini.py
SUPPORTS_VICAR = True
SUPPORTS_FITS = False

# instruments/__init__.py
ALL_INSTRUMENTS = [cassini, voyager, galileo, hst, nh]
VICAR_INSTRUMENTS = [i for i in ALL_INSTRUMENTS if i.SUPPORTS_VICAR]
FITS_INSTRUMENTS = [i for i in ALL_INSTRUMENTS if i.SUPPORTS_FITS]

Option B keeps the named filtered lists (better for discoverability), at the cost of every instrument module needing the two flags (vs. just letting detect_vicar/detect_fits return None).

Recommended

Option A — the detect_* predicates are already the single source of truth (e.g., cassini.detect_fits is unconditionally None). Storing the same information in a second place (VICAR_INSTRUMENTS) is what allows drift. The minor perf cost of calling no-op detect_fits once per cascade per instrument is invisible (~µs).

Acceptance criteria

  • VICAR_INSTRUMENTS / FITS_INSTRUMENTS removed from src/picmaker/instruments/__init__.py.
  • src/picmaker/io.py iterates instruments.ALL_INSTRUMENTS for both VICAR and FITS probes.
  • Existing test suite passes unchanged.
  • Adding a new instrument now requires editing exactly one list (ALL_INSTRUMENTS) plus the new module file.

Related

  • CODEBASE_CRITIQUE.md §1 — original finding.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions