Extract a `plot_types/` package — one module per plot type

[AlexisJanin]

Rewritten after a grilling session (2026-08-26). The original scope — consolidate two duplicate lists and the hovertemplate if/elif — was too small. Measuring first found ~830 lines of per-plot-type code interleaved into five shared modules, and six spellings of the derived-section list rather than three. Full design record: .scratch/architecture/10-plot-types-package.md.

The decision

A plot type is a module. Everything that varies by plot type lives in that type's package; no other module branches on plot type.

Why — the measurement

PSD touched 7 src files / +680 lines with the PlotType abstraction already built, and still missed a site. The code records the miss itself, at other/find_load_format.py:112:

Adding a fifth derived plot type means adding a row here -- forgetting to is what made 'psd' validate cleanly yet never render.

That is the failure mode to design against: a plot type that passes validation and renders nothing.

Per-plot-type code, currently interleaved into shared modules:

Module	Total	Per-plot-type	Share
database_options_xlsx.py	657	~248 (3 sheet blocks + 2 psd-only helpers)	38%
database_options_parser.py	379	~128 (_check_spectral_types, _check_psd_entries, the pair)	34%
plot_assembly.py	526	~137 (builders, qualifiers, _DERIVED_PLOTS)	26%
signal_container.py	1253	~285 (3 factories, heatmap branch, hover if/elif)	23%
other/find_load_format.py	—	~36 (PER_FILE_DERIVED_SECTIONS + 3 qualifiers)	—
spectral.py	230	230 (already isolated)	100%

Six spellings of the same list, three memberships. loop is absent from the parser's two because it has no config class — grep -n loop database_options_parser.py returns one hit, and it is the word "loop" in a comment. loop has zero config validation today.

1. constants.py:720-726 — the import-time guard (LOOP, SPEC, PSD)
2. plot_assembly.py:278-291 _DERIVED_PLOTS — build + qualify + refusals
3. other/find_load_format.py:114-118 PER_FILE_DERIVED_SECTIONS — qualify
4. database_options_parser.py:104-106 (SPEC, PSD) — config class
5. database_options_parser.py:135-139 (SPEC, PSD) — config class
6. database_options_xlsx.py:38-45 + three reader blocks (LOOPS, SPECTROGRAMS, PSDS)

Sites 2 and 3 are the same three shape-walks with a different leaf op — resolve-then-prefix datasource:: vs blind-prefix stem::. Six functions, three shapes, two leaf ops; the duplicated copy is the one forgotten for PSD.

Layout

Mirrors datasource/sources/<name>/ exactly — options.py / find_load_format.py becomes schema.py / plot.py.

plot_types/
  base.py          PlotTypeSchema + PlotTypeBase  (defaults ARE time_series)
  registry.py      AVAILABLE, PAGE_ORDER, import-time completeness guard
  loop/        schema.py + plot.py
  spectrogram/ schema.py + plot.py
  psd/         schema.py + plot.py

The split is by import-reachability, not by "declarative vs machinery".

• schema.py — what every layer may import: name/section_key, config keys, validation, map_refs, xlsx sheet spec + row interpretation, and the five capabilities (pure booleans, read by signal_container, hence in the leaf half).
• plot.py — what only the top of the stack may: build(), the maths, the installed rendering. Imports signal_container and spectral.

time_series is not a package. It is the substrate: one registry line, PlotTypeBase(name=...), inheriting every default; derived = [t for t in AVAILABLE if t.section_key]. Precedent for a registered-but-degenerate member: OtherDataSource stubs _load/extract and stays in DataSource.AVAILABLE. The capability table already encodes this — every default is time_series's behaviour and each derived type is a small delta:

Capability	time_series	loop	spectrogram	psd
time axis	✓	✗	✓	✗
unified hover	✓	✗	✗	✗
resampled	✓	✗	✗	✗
grid layout	✗	✓	✗	✗
colorbar	✗	✗	✓	✗

The constraint that shapes everything: a real import cycle

signal_container.py:16 imports datasource.formatting.timezone; that executes datasource/__init__.py → registry.py → every source module → other/find_load_format.py, which imports signal_container. That loop exists today and survives only because Python tolerates submodule imports of a half-initialised package.

plot.py needs Signal to build one. So signal_container must never import a plot.py — import signal_container first and it stops at its own import line, before class Signal exists, and the from … import Signal inside plot.py raises ImportError. Not the tolerable case; a hard failure whose occurrence depends on the entry point.

Hence: capabilities live in schema.py, and rendering is pushed, not pulled.

Decisions

1. Signal becomes type-agnostic. loop_from_signals, spectrogram_from_signal, psd_from_signal (~215 lines) move out of Signal into their type's plot.py. signal_container.py 1253 → ~970.
2. Layout mirrors datasource/sources/ — package per type, two files.
3. Registry holds all four; time_series is degenerate. The five capability tuples leave constants.py; all ten read sites go through the registry. Rejected: capabilities staying in constants (a new type would declare behaviour in two places); fallback-to-defaults for unregistered types (a typo'd string would silently get time_series behaviour — the same silent acceptance this issue exists to kill).
4. xlsx: the reader transcribes, the plot type interprets. Sheet name, required columns and row→config mapping move to schema.py; _read_optional_sheet, _is_empty, _to_float, _is_truthy and file-level orchestration/error reporting stay in database_options_xlsx.py. Same seam as ADR-0010, one layer up; exact line is an implementation judgement. Tiebreaker: the spreadsheet columns and the JSON keys are one schema in two spellings (psds requires freq_min/freq_max because PsdConfig requires freq_range), and splitting them across modules is what lets them drift — in the format clinicians actually author.
5. Trace construction: template method, two hooks. Base builds the Scatter and applies the hover spec; loop and psd override hover only; spectrogram overrides trace construction outright (a go.Heatmap is a different Plotly primitive, not a Scatter variant). The ~50 shared lines (timezone conversion, line/marker dicts, unit suffixes, hover_formatters keyword path, legendonly) are shared by nature — they are about TraceOptions, not about plot type.
6. Push, not pull. build() installs rendering on the Signal it returns — a HoverSpec(template, customdata) and, for spectrogram, a trace factory. to_plotly_trace precedence becomes: user's hover_template → keyword formatter → installed spec → None. Strictly one-way dependency, no lazy imports. Rejected: pull plus a function-local import Signal in each build() — the cycle would then be held off by an unwritten convention, invisible at the point of violation, failing as a non-deterministic ImportError; the same bug class being fixed. A plain time_series Signal needs no installation — Signal's own defaults are its behaviour.
7. Nothing plot-type-related stays in constants.py. Verified: the file's only PlotType reference is the guard at :720-726, which is deleted (DEFAULT_LOOP_SUBPLOT_HEIGHT / LoopSubplotHeight only look related — "loop_subplot_height" is a user-settings key string). SpectrogramConfig/PsdConfig move to their schema.py, following datasource/sources/<name>/options.py: constants.py holds cross-cutting literals, a module's own option schema lives with the module. KNOWN_SECTION_KEYS declares only non-derived keys and the parser unions {s.section_key for s in schemas}, so a new plot type can no longer trigger "Unknown key" warnings on a valid config. cst.Spectral stays — it is the signal-analysis domain shared by two plot types, the relation io/ has to datasources.

Deliberately no ADR. The datasource module layout has none either — ADR-0008 records the genuine judgement call (module vs other/), not the layout, which the folder and /new-datasource teach on their own. The one rule here with a revivable alternative (push vs pull) is enforced by a test instead of a document.

Delivery — two commits, one PR, branch off main

Split along the two-file seam, so staging costs nothing extra to design and carries no transitional duplication:

	Moves	Guarded by	Touches
1 — plot.py	builders, trace, hover, capabilities, base + registry	snapshot suite	signal_container, plot_assembly, 2 dash callback files, constants.PlotType capabilities
2 — schema.py	config classes, validation, xlsx interpretation, map_refs	config/validation tests	database_options_parser, database_options_xlsx, other/find_load_format, constants.DatabaseOptions, ValidationIssue move

Each stage has one dominant failure mode with one suite that catches it. Stage 1 is the risky half and carries the visible win; stage 2 is mechanical.

Consequences to handle

• ValidationIssue must move to a leaf. A 3-field NamedTuple in database_options_parser.py; a schema.py returning issues would cycle. ~6 lines plus two import sites (wrapper.py:12, data_callbacks.py:37).
• PAGE_ORDER moves to plot_types/registry.py — an ordering across types belongs to the collection. Same deviation from CLAUDE.md's "orderings in constants.py" that DataSource.AVAILABLE already makes, for the same reason.
• other's loop walker crashes on a non-list config where plot_assembly's returns it unchanged; other's spectrogram walker copies where plot_assembly's aliases. Unifying takes the guarded version — a small behaviour change in the safe direction: a malformed per-file loop currently skips the whole file, afterwards it is skipped as one bad plot.
• CLAUDE.md's "Where things live" tree and Architecture section need a plot_types/ entry.

Acceptance criteria

• No module outside plot_types/ branches on plot type or hardcodes a derived section key.
• signal_container imports no plot_types/*/plot module (asserted by test).
• Adding a plot type = one package + one registry line. Nothing in constants.py, database_options_parser.py, database_options_xlsx.py, other/find_load_format.py, plot_assembly.py or signal_container.py changes.
• A forgotten piece is an import-time crash, never a config that validates and renders nothing.
• loop gains the config validation the other two have.
• Snapshot tests unchanged — no figure moves.

Tests

• Fake fourth plot type (tests/plot_types/fake/), registered by a fixture, driven through all six paths: validates, warns on a bad key, scopes references inside an other::<stem> block, reads an xlsx sheet, builds, renders. ~100 lines, milliseconds, no committed fixture — xlsx_bytes_to_database_options(bytes) already exists (database_options_xlsx.py:622), so the workbook is built to a BytesIO at test time. Shaped deliberately unlike the real three (a config section whose value is a bare string) so it exercises map_refs on an unused shape.
• AST boundary test, in the style of tests/datasource/test_load_config_independence.py: no module outside plot_types/ compares against a plot type string or hardcodes a section key, and signal_container imports no plot_types/*/plot. This replaces the ADR for decision 6, and guards the two Dash callback files — the least-tested layer, and the easiest place for a type branch to reappear.
• Import-time completeness guard in plot_types/registry.py: every member has both halves, section_key matches its package name, capabilities declared. Inherits the deleted constants.py guard's job.

Relations

Split from #81; the :: half of that section is deferred to #80. Follow-ups opened alongside this rewrite: collapsing other's per-file scoping into plot_assembly, and a /new-plot-type skill.

[AlexisJanin]

Implemented — branch plot-types-package, two commits

• 72ce4b3 — Move each plot type's rendering into a package of its own (the plot.py half)
• 2fbf6dc — Move each plot type's config into its own package (the schema.py half)

Verification: 1060 tests pass; ruff check src/ and ruff format --check src/ clean. The snapshot suite is unchanged — no figure moves. The demo database_options.json still regenerates byte-identical from its .xlsx.

Acceptance criteria

• No module outside plot_types/ branches on plot type or hardcodes a derived section key — swept across all 74 src/ modules by tests/plot_types/test_boundaries.py. No exemption was needed in the end: constants.py no longer names a plot type either.
• signal_container imports no plot_types/*/plot module (asserted by test).
• Adding a plot type = one package + registry lines; none of the six shared modules change.
• A forgotten piece is an import-time crash — registry guard (find_spec on the missing plot.py) plus a builders guard.
• loop gains the config validation the other two have.
• Snapshot tests unchanged.

Tests

• Fake fourth plot type — tests/plot_types/fake/, registered by a fixture, driven through all six paths. Its config entry is a bare string, a shape none of the real three use. Workbook built to a BytesIO at test time, no committed fixture.
• AST boundary test — tests/plot_types/test_boundaries.py, in the style of test_load_config_independence.py.
• Import-time completeness guard in plot_types/registry.py.

Sizes

Module	Before	After
signal_container.py	1198	943
database_options_xlsx.py	657	460
constants.py	727	636
plot_assembly.py	526	285
database_options_parser.py	379	260

1679 lines now live in plot_types/.

Deviations from the plan

1. Two registry lines, not one. The plan's "capabilities live in schema.py so signal_container reads them freely" and "PAGE_ORDER moves to registry.py" are only jointly satisfiable if the registry itself never touches a plot.py — otherwise signal_container → registry → psd/plot → signal_container is exactly the ImportError the design exists to prevent. So the registry is split: registry.py imports schemas only (leaf, safe for every layer), builders.py imports the plot halves and is read by plot_assembly alone. Adding a type is a package plus a line in each; both lines are inside plot_types/, and omitting either crashes at import.

2. signal_reference.py — a new leaf. Builders resolve the references in their own config, but _resolve_signal_references lived in plot_assembly, which now imports the builders. The 3-mode lookup moved out unchanged (ADR-0013 behaviour untouched); plot_assembly and every plot.py import it from there. This changes CLAUDE.md's line naming plot_assembly._resolve_signal_references — updated.

3. A sixth capability, POINT_TIMESTAMPS. The plan listed five, but two Dash sites compared == LOOP outright (annotation_callbacks.py:411, data_callbacks.py:1394). Both mean "every drawn point carries the instant it was recorded, although x is not time" — the annotation reads it from hover customdata, the time slider from loop_time_axis. Without it, acceptance criterion 1 could not hold.

4. CellReader for the xlsx seam. A schema.py cannot import database_options_xlsx (the reader imports every schema to find its sheet), so the four cell coercions stay exactly where the plan put them and are lent to a schema for the length of one sheet. It states the seam out loud: the reader owns how a cell is read, the plot type owns what a row means.

5. DatabaseOptions.LOOP / SPECTROGRAM / PSD deleted outright. Implied by decision 7 but not stated. With KNOWN_SECTION_KEYS no longer declaring them and the config classes moved, nothing outside a schema had a reason to name one — and deleting them is what lets the AST sweep run over constants.py with no exemption.

Found while implementing

The fake plot type paid for itself before it was finished: plot_assembly did from ...builders import BUILDERS, binding the dict by value at import, so the module held a second source of truth for a collection the registry owns. Invisible in production (the dict is never mutated at runtime), instant KeyError under a runtime-registered type. Two more import-time snapshots went the same way — _KNOWN_SECTION_KEYS in the parser and _DERIVED_PLOTS in assembly are now computed per call.

Behaviour changes to note in review

• loop validation is new, so a config carrying a broken loop starts reporting it where it was silent. Nothing that drew before stops drawing — the shipped demo config validates completely clean. Changelog entry added.
• A malformed per-file loop now costs one plot, not the whole file — as predicted in the design record. Scoping it used to assume a list and raise inside other's per-file handler, taking every signal in that file with it. Pinned by a test in tests/datasource/test_other.py.

Note on the base branch

Branched off code-quality-improvement, not main: plot_assembly.py does not exist on origin/main (ADR-0013 / 7079055 has not merged), so main is not a base this plan can be implemented against.

Follow-ups now unblocked

#91 (collapse other's per-file scoping into plot_assembly) and #92 (a /new-plot-type skill). Both untouched here, as planned.

[AlexisJanin]

Where delivery diverged from this design

Recording it here rather than editing the issue, since the body is the pre-implementation design record. Delivered in #93; the acceptance criteria are all met, three of them by a different mechanism than described.

"The constraint that shapes everything: a real import cycle" — the constraint is gone, not routed around.

The design was built on signal_container never importing a plot.py, because it is reachable from a half-initialised datasource package. That was true of the dependency graph, but it never fired: clinical_scope/__init__.py eagerly imports wrapper, which fixes module order for every entry point, and signal_container bound the registry as a module object, touching attributes only at call time.

The fix was not to route around it but to remove the edge. PlotOptions and PlotModel now carry the definition class, so every capability read is self.definition.GRID_LAYOUT rather than a registry lookup keyed by name. signal_container imports only plot_types.base.

So decision 6 ("push, not pull") generalised rather than staying a special case. The design pushed rendering via RenderSpec while capabilities stayed pulled — two answers to one question, and the pulled one accumulated hand-maintained duplicates. PlotOptions.square_plot was a second spelling of GRID_LAYOUT kept in sync by nothing: the fake plot type declared one and not the other and rendered full-width. Both are pushed now, and square_plot is deleted.

Decision 3's rejected alternative held up. Capabilities did leave constants.py and a typo'd name still gets no behaviour — but via an Unknown definition with every flag off, rather than the type simply being absent from six frozensets.

Consequence for the two-file split. The design justified it by import-reachability; with the cycle gone that argument went too, and the config half was measured to be unreachable alone anyway (import clinical_scope.database_options_parser already loads plotly, numpy and all three plot.py). The split was kept on readability plus the parallel with datasource/sources/<name>/ that this issue drew in the first place. registry.py / builders.py, split for the same dead reason, were merged.

schema.py is definition.py. The file holds identity and six rendering capabilities as well as config grammar, and the codebase already spends "schema" on UserOptions classes and the Dash widget registry. PlotTypeSchema → PlotTypeDefinition, plot_options.schema → .definition.

Two small corrections to the issue's own text, for anyone reading it later: the capability table lists five, and there are six — POINT_TIMESTAMPS was added with the loop time-slider. And "adding a plot type = one registry line" is two adjacent lines, AVAILABLE and BUILDERS, both now in registry.py.

Deliberately no ADR, as the issue asked. One was drafted for the capability push and rejected on review: it described how the code works rather than a decision someone could reasonably have made otherwise, which is what a module docstring is for.