From 9b79eef9a2e8072531d338e1b42cfaebd99c93b9 Mon Sep 17 00:00:00 2001 From: jbloom Date: Fri, 30 Oct 2026 08:17:47 -0700 Subject: [PATCH 1/4] keep selection params' views when loading a chart from a spec A chart with a selection param on a compound chart (layer, facet, concat) failed to render when passed to `plot()` as a JSON path, HTML path or dict, i.e. on every CLI call: Vega-Lite reported `Duplicate signal name: "_tuple"`. `_load_chart` deleted `views` from every param before `alt.Chart.from_dict`. When `_build` then nests the chart, altair keeps a compound sub-chart's existing `views`, so the stripped params came out as `views: []`, and Vega-Lite defines such a selection in every view. Live charts never went through the strip, which is why the docs assets and downstream Python users (e.g. flu-seqneut-2026) never hit it. Why the strip existed: phase 2b (0812a7b) found that nesting a chart's *dict* inside a dict-level concat fails validation with "'views' was unexpected". `views` is only valid on top-level selection params, and nested params are validated against the nested schema. Phase 2b avoided dict round-trips altogether. Phase 2c (ded7beb) reused that note verbatim for the new loader and stripped `views` before a *top-level* `from_dict`, where they are valid. Reproducing it with the period chart JSON and the altair installed then (6.1.0): - top-level `from_dict` accepts `views`; - only the dict-nesting approach rejects them; - stripping doesn't rescue that approach either ("'select' is a required property"). The docstring's "the renderer rebinds params to views on its own" is not true for an emptied list. No test rendered JSON-path output, and the phase 2c and current suites both pass unchanged with the strip disabled. Removing it is safe on every supported altair. On 6.0.0, 6.1.0, 6.2.2 and 6.3.0: - with the strip, only unit charts (2 of 12 shape x tree-side cases) render; - without it, all 12 do. So the `altair>=6.0,<7` floor stays. The `deepcopy` stays: `from_dict` aliases the input's `datasets` row lists. The new regression test passes dict and JSON-path charts of each compound shape, and checks both that every selection keeps `views` naming real views and that vl-convert renders the result. It fails on the old loader in all 10 cases. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 11 ++++ CLAUDE.md | 12 +++- src/tree_annotated_plot/_plot.py | 35 ++++------ tests/test_chart_loader.py | 108 +++++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf702ae..93d7f68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- A chart with selection parameters on a compound chart (layer, facet, or + concat) failed to render when passed to `plot()` as a JSON path, HTML path + or spec dict, which is every CLI use. Vega-Lite reported + `Duplicate signal name: "_tuple"`. This affected the flu examples' + CLI reproductions. The chart loader removed each selection's `views` before + loading, and altair then emitted empty `views`, which makes Vega-Lite define + the selection in every view. The loader now leaves the spec's `views` intact. + Live altair charts were never affected. + ## [0.4.0] - 2026-08-20 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index f9fa2fe..632e1dd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -52,6 +52,13 @@ on `x` or `y`, overrides its sort to match the tree's tip order, and hconcat's or vconcat's a tree panel on the matching side. Tree dictates tip ordering; plot follows. +- **Loaded specs go to `from_dict` unmodified.** Don't strip or rewrite + altair's annotations when loading a dict / JSON / HTML chart. Selection + params' `views` in particular are valid at the top level; removing them made + every compound chart with a selection fail to render ("Duplicate signal + name"). See `_load_chart`. Likewise, declare extra params by appending to + the combined chart's top-level `params`, never with `add_params`, which + recurses into sub-charts and raises on spec-loaded ones. - **Two surfaces, one config**: `tree_annotated_plot.plot()` and the `tree-annotated-plot` CLI both accept the same parameters via `PlotConfig`. They converge on `_build(tree, chart, config)` so they can never disagree. @@ -78,7 +85,10 @@ - **Eyeball examples after any rendering change.** There are no image-snapshot regression tests (vl-convert/font/platform variance makes them flaky), so visual regressions only get caught by - looking. After any change to `_plot.py`, `_tree.py`, the example + looking. Tests may still render with `vl_convert.vegalite_to_svg` + to check that a spec renders at all, or to assert discrete SVG + attributes such as a label's `font-weight` / `fill`; those are + deterministic. After any change to `_plot.py`, `_tree.py`, the example scripts, or anything they touch: run `scripts/build_docs.sh`, open `site/examples.html`, and click through each example — both the embedded SVG and the interactive HTML link. Confirm tip-row diff --git a/src/tree_annotated_plot/_plot.py b/src/tree_annotated_plot/_plot.py index f3639e8..0432d62 100644 --- a/src/tree_annotated_plot/_plot.py +++ b/src/tree_annotated_plot/_plot.py @@ -390,9 +390,15 @@ def _load_chart(chart_input: ChartInput, *, strict_version: bool) -> alt.TopLeve A live `alt.TopLevelMixin` is returned as-is — its `$schema` is irrelevant since the constructing altair version is necessarily the running altair version. For dict / JSON path / HTML path inputs we run - the Vega-Lite version check (under `strict_version`), strip altair's - `params[].views` round-trip annotations, and then dispatch to the right - chart subclass via `alt.Chart.from_dict(...)`. + the Vega-Lite version check (under `strict_version`) and then dispatch to + the right chart subclass via `alt.Chart.from_dict(...)`. + + The spec goes to `from_dict` unmodified. In particular, each selection + param's `views` must survive: when `_build` nests a compound chart (layer + / facet / concat), altair keeps its params' existing `views`, and an + emptied list makes Vega-Lite define the selection in every view, which + fails with "Duplicate signal name". `views` is valid on top-level params, + which is the only place altair emits it. """ if isinstance(chart_input, alt.TopLevelMixin): return chart_input @@ -420,8 +426,9 @@ def _load_chart(chart_input: ChartInput, *, strict_version: bool) -> alt.TopLeve ) _check_chart_schema_version(spec, strict_version=strict_version) + # `from_dict` keeps references into the spec (e.g. the `datasets` row + # lists), so copy it to keep the caller's dict out of later mutation. spec = copy.deepcopy(spec) - _strip_params_views(spec) # `from_dict` with validate=True dispatches to the right subclass # (Chart / LayerChart / FacetChart / HConcatChart / VConcatChart / # ConcatChart) based on the spec's shape. @@ -485,26 +492,6 @@ def _check_chart_schema_version(spec: dict, *, strict_version: bool) -> None: ) -def _strip_params_views(spec: Any) -> None: - """Recursively delete `views` from every params[] entry. - - altair's `to_dict()` annotates each selection-param with the views it's - bound to, but altair's own `from_dict()` validation rejects the field. - Stripping is safe: the renderer rebinds params to views on its own. - """ - if isinstance(spec, dict): - params = spec.get("params") - if isinstance(params, list): - for p in params: - if isinstance(p, dict): - p.pop("views", None) - for v in spec.values(): - _strip_params_views(v) - elif isinstance(spec, list): - for item in spec: - _strip_params_views(item) - - class _ScriptCollector(HTMLParser): """Collect the text content of every