Extract a plot_types/ package — one module per plot type (#83, #91)

[AlexisJanin]

Implements #83 (a plot_types/ package, one module per plot type) and its follow-up #91 (other stops knowing what a spectrogram is).

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.

The failure mode being designed against is the one PSD hit: a plot type that validates cleanly and renders nothing, because one of six spellings of the derived-plot list was forgotten. Those six spellings are now one registry.

What moved

From	To
Signal.loop_from_signals / spectrogram_from_signal / psd_from_signal	plot_types/<name>/plot.py
plot_assembly's builders, qualifiers and _DERIVED_PLOTS	plot_types/registry.py + each definition.py
constants.PlotType, its capability tuples and its import-time guard	plot_types/registry.py
constants.SpectrogramConfig / PsdConfig	plot_types/<name>/definition.py
database_options_xlsx's three sheet blocks + two psd helpers	each definition.py
database_options_parser's _check_spectral_types / _check_psd_entries	each definition.py's validate()
other/find_load_format.PER_FILE_DERIVED_SECTIONS + its 3 qualifiers	deleted — plot_assembly desugars both scopes (#91)
ValidationIssue	validation.py, a leaf both halves can import
reference resolution	signal_reference.py
PlotModel.assign_plot_model	plot_assembly.assemble_plot_models

Everything a plot type knows travels on the object

A Signal carries its definition (plot_options.definition) and its RenderSpec. Every render site reads the flag off that object:

if self.definition.GRID_LAYOUT:      # not: if self.plot_type in registry.GRID_LAYOUT

So signal_container imports nothing from plot_types but base, and the data model never knows the roster. plot_type survives as a read-only property returning definition.NAME, for logs, figure titles and Dash stores.

registry.definition_for(name) is the single exception, at the one boundary where the object could not be carried: a plot type that crossed a Dash store as JSON has only its name left. An unregistered name resolves to Unknown, every capability off — deliberately not the time_series defaults, which would let a typo render plausibly.

This replaces what the first four commits did, and reverses a conclusion this PR previously stated. They kept capabilities as registry-side frozensets keyed by name, because signal_container is reachable from a half-initialised datasource package and so must not import a plot.py. That constraint was real on the dependency graph but 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. Both facts were already visible in the earlier measurement — "Both designs import fine through the normal entry point" — which produced its ImportError only after separately modifying the tree to settle the datasource cycle first.

The cost was never the cycle. It was that one question had two shapes — rendering pushed, capabilities pulled — and the string-keyed one grew hand-maintained duplicates. PlotOptions.square_plot was a second spelling of GRID_LAYOUT kept in sync by nothing: the fake plot type in the test suite declared one and not the other, and rendered full-width.

The two halves per package

• definition.py — what the type is: NAME/SECTION_KEY, the six capability flags, config keys, validate(), map_refs(), xlsx sheet + row interpretation. Imports nothing but validation.
• plot.py — what it does: build(), the maths, the rendering it installs. Imports signal_container, numpy and plotly.

The split is stylistic, not load-bearing — reversing this PR's earlier claim. With the cycle gone the layering argument went too, and it was measured rather than assumed:

>>> import clinical_scope.database_options_parser
heavy modules pulled in: ['plotly', 'numpy']
plot.py modules loaded : [loop.plot, psd.plot, spectrogram.plot]

No entry point ever saw a config layer that loads without plotly. What the split earns now is readability and consistency: psd is 302 lines of spreadsheet row accumulation and group resolution against 154 lines of a Welch call and axis wiring, and it keeps the shape a registered module already has here — datasource/sources/<name>/ is options.py plus find_load_format.py, the same declarative half and working half.

registry.py and builders.py, split for the same dead reason, were merged. time_series is registered but has no package — every default in PlotTypeDefinition is its behaviour, and DERIVED is the types with a SECTION_KEY.

Behaviour changes

• loop gains config validation. It had none — wrong arity, a non-list entry and a non-string member were all silent, and a loop naming one signal simply failed to appear.
• A malformed per-file other::<stem> loop now costs one plot instead of the whole file.
• spectrogram and psd now report a non-dict entry instead of returning no issue, which was the validates-cleanly-renders-nothing failure surviving inside the package. loop already reported it.
• Two Dash sites that compared == LOOP now read a sixth capability, POINT_TIMESTAMPS.
• KNOWN_SECTION_KEYS declares only the keys no plot type owns; the parser unions each registered type's own key onto it, so a new plot type can no longer make a valid config warn "Unknown key".
• A grid plot type with a single subplot now gets its square width from GRID_LAYOUT. Loops always set both flags, so nothing visible changes for them — but a future grid type can no longer half-declare itself.

Acceptance criteria

• No module outside plot_types/ branches on plot type or hardcodes a derived section key — asserted by tests/plot_types/test_boundaries.py (AST).
• signal_container imports nothing from plot_types but base — same test, strengthened from the original "imports no plot.py".
• Adding a plot type = one package + two adjacent lines in registry.py (AVAILABLE and BUILDERS). Qualified — see Scope of the "nothing changes" claim.
• A forgotten piece is an import-time crash, never a config that validates and renders nothing — tests/plot_types/test_fake_plot_type.py registers a fourth type and drives it through all six paths.
• loop gains the config validation the other two have.
• Snapshot tests unchanged — no figure moves.
• other/find_load_format.py imports nothing from plot_types (Collapse other's per-file reference scoping into plot_assembly #91).
• Both config spellings stay valid — no parser change, no config migration.

Scope of the "nothing changes" claim

True of how a plot type behaves. A type wanting a user display setting still needs a UserOptions class in constants.py plus a DisplayFallbacks field (as loops_per_row and spectrogram_db_range do), and one wanting an axis payload of its own needs a field on Data (as point_time_axis and spectrogram_freq_axis do). FakeDefinition has neither, so the test proving the claim does not cover that half. CLAUDE.md and registry.py say so.

Why definition and not schema

"Schema" described one third of the file. Alongside config grammar it carries the type's identity and its six capability flags — booleans about how the type draws, and the file's most-read content. The word was also already spent twice: cst.UserOptions.LoopsPerRow and its siblings are schema classes, and data_callbacks builds a Dash schema-registry mapping component ids to widget schemas. Three meanings; the plot type had the weakest claim.

self.definition.GRID_LAYOUT reads as "this plot type's definition says it is a grid", where self.schema.GRID_LAYOUT read as a category error. The rename was not applied blindly — the other two meanings were left alone, and sentences it would have degraded were rewritten ("the sheet columns and the JSON keys are one schema in two spellings" was right the first time, and became "one grammar in two spellings").

Commits

1. 52fcad7 — rendering into a package of its own (the risky half; guarded by the snapshot suite)
2. e708a44 — config into its own package (mechanical; guarded by the config/validation tests)
3. 98dffe7 — other stops referencing plot types (Collapse other's per-file reference scoping into plot_assembly #91)
4. 01d83a8 — three-axis review follow-ups
5. c0c349b — guard the periphery a plot type lands in (demo config + docs)
6. ef8297f — a /new-plot-type skill
7. 6e44a34 — comments stating current behaviour, not what the branch changed
8. ca9b724 — capabilities travel on the object
9. 9a39124 — fold the builders back into the registry
10. c9164de — rename the schema half to the definition half

Review follow-ups (ca9b724 onward)

A fourth review pass, on readability and maintainability rather than correctness. Verified against the code before being acted on:

• data_callbacks read plot_model.name for RESAMPLED while reading plot_model.plot_type for POINT_TIMESTAMPS. They agreed only because __post_init__ aliases name to the plot type — and name is also a Dash component id, so giving a PlotModel a real title would have silently broken resampling.
• Data.loop_time_axis → point_time_axis, matching the POINT_TIMESTAMPS capability that gates it. data_callbacks reads it inside a branch on that generic flag, so a second timestamped type would have had to populate a field named after loops.
• The schema/builder seam was typed Any throughout. PlotBuilder.build and read_sheet now have real signatures behind if TYPE_CHECKING, which never executes and so closes no cycle. entries/entry/config stay Any and say why: raw user JSON, and narrowing them is validate()'s job.
• CellReader gains text() and pair(), absorbing 15 str(row.get()).strip() copies and the db_min/db_max rule that was written twice with two different messages. ValidationIssue.unknown_keys replaces six hand-rolled copies of one message. The row loop is deliberately not unified — psd's rows fan out into several groups, so a shared template would have one and a half users.
• The comments in 6e44a34 were the same class of finding: several stated what the branch changed ("which is what let psd validate cleanly and render nothing before this") rather than what the code does, which is unreadable to anyone who did not review the diff.

Known, deliberately left

• A malformed derived section (loop: [], as opposed to a malformed entry) still raises into _flatten_config's handler and skips the whole namespace, taking its grouped_fields with it. Matches pre-existing behaviour, so not a regression — worth its own change.
• Four maintainability items from the last review, none affecting behaviour: _add_derived_plot_group takes six parameters for one call site; PlotTypeArityError and SourceSignalNotFoundError live in plot_types/base but are raised by loop/plot.py and signal_reference; DisplayFallbacks still carries three type-named fields; registry still flattens AVAILABLE into five collections, which the fake-type fixture restates.

Verification

1107 tests pass, ruff clean. Snapshot tests are in that count (16, not deselected), so no figure moved. The demo database_options.json still regenerates byte-identical from its .xlsx.

The one changed area with no automated coverage is data_callbacks._build_graphs — its only caller is process_visualization, whose test never reaches it. The resampler wrap, the loop time-range slider and a single-subplot grid plot were checked by hand in the running app.

🤖 Generated with Claude Code