Repository navigation
Conversation
…on its own frame (#471)
…e real selection names (#471) Two review corrections: - filter_selection_by_unit() raises the actionable fields=/encodings= error for an IndexSelection before partitioning store entries, so a bare point selection never surfaces the confusing unknown-unit error. - Selection fixtures in the new selected_rows tests now carry their param's name (as altair's JupyterChart.selections always emits), so the tests exercise the primary hoisted-param view resolution rather than silently taking the store-unit fallback.
- view_frames() (replacing named_subcharts()/view_frame()) resolves each view to the nearest data frame declared on its subchart or an ancestor, so a view whose data is hoisted onto an intermediate compound chart no longer raises or falls back to the wrong root frame; facet specs are walked explicitly. - A bare point selection (IndexSelection) on any chart with several datasets now raises the fields=/encodings= error even when it participates in a single view or a scalar df is passed: Vega's _vgsid_ ids are global to the chart's dataflow, so they aren't row positions of any one view's frame. - A scalar df= is accepted again when a multi-view selection's views all share the identical data frame object (restoring pre-PR behavior).
59a90c5 to
a9d3384
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved data-source handling can incorrectly reuse a parent frame and permit unsafe positional selections across mixed sources, causing selected_rows() to return incorrect or empty results.
2 open issues
3 resolved since last review
Review effort: Balanced
…ds (#471) Addresses a second round of Copilot review: - view_frames() inherits a parent's data frame only when a subchart declares no data of its own. An explicit URL or inline alt.Data source now resolves to None (requiring df=) instead of silently filtering the parent frame, and the single-view selected_rows() path no longer substitutes the chart's root frame for an unresolved view. - chart_datasets() counts every distinct data source (frames by identity, URLs by value, anything else explicit by identity), so a bare point selection on a mixed frame/URL chart also raises the fields=/encodings= error instead of treating global _vgsid_ ids as row positions.
|
@copilot please re-review |
Re-review found two regressions:
The current PR CI checks pass. |
…471) Addresses a third round of Copilot review (two regressions vs main): - A selection spanning a repeat chart's generated views failed even with a scalar df=. Views view_frames() can't name now resolve to the chart's own frame: chart_frame() descends into a repeat chart's spec (repeated subcharts share one dataset), and chart_datasets() counts data declared on the spec. - Views sharing a single non-frame dataset (e.g. subcharts over one URL) rejected a scalar df= that main accepted. A scalar df= now stands in whenever the whole chart declares exactly one dataset.
|
@copilot please re-review |
Re-review found a remaining silent mis-filter in The latest CI run is still in progress; the preceding run passed. |
…-dataset chart (#471) Addresses the latest Copilot re-review: a nested repeat that declares its own data frame inside a concatenation with root data had its generated views resolved to the root frame, so a brush on the repeat silently filtered the wrong rows. Views view_frames() can't name now resolve to the chart's frame only when the whole chart declares exactly one dataset; otherwise selected_rows() raises, pointing at df=.
|
@copilot please re-review |
Re-review found one remaining mis-filter: in All Python and Playwright CI jobs passed on the current commit. |
…#471) Addresses the latest Copilot re-review: - selection_views() missed selections on charts built with alt.Chart.from_dict, whose parameter names are schema objects (ParameterName) rather than plain strings. Names are now unwrapped via to_dict(). - When selection participation is unreadable and selected_rows() falls back to the store's units, a scalar df= on a chart with several datasets was filtered against whatever units happened to report entries -- a silent mis-filter when the selection participates in more views than the store shows. It now raises, pointing at a dict df= keyed by view name.
|
@copilot please re-review |
Re-review found two remaining cases:
All Python and Playwright CI jobs passed on the current commit. |
…lti-dataset charts (#471) Addresses the latest Copilot re-review: - An empty views list on a selection param doesn't mean the selection participates in no views -- Vega applies it to any view. Treat it as unreadable participation (store fallback) instead of filtering the chart's root frame. - The store-unit fallback now rejects multi-dataset charts outright (with or without a scalar df=): the store only shows views with active selections, so results were incomplete and their shape depended on interaction state. The error points at a dict df= keyed by view name.
|
@copilot please re-review |
Re-review found one compatibility regression in CI’s ipydatagrid lifecycle test failed with a transient view count of 6 rather than 5; a retry is already in progress. The Python jobs passed. |
Addresses the latest Copilot re-review: two alt.NamedData references to the same dataset (including on imported charts) were counted as distinct sources by identity, so a shared-source hconcat rejected a scalar df= that worked before multi-dataset support. Named references are now deduplicated by dataset name, like URLs are by value.
❌ Deploy Preview for shinywidgets failed.
|


Fixes #471.
Summary
@render_altair'sselected_rows()silently mis-filtered selections whose Vega store spans several views: on a layered or concatenated chart whose subcharts use different data frames, every view's selection entries were applied to a single frame, returning the wrong rows when the datasets shared column names (or raising when they didn't).Now the store is partitioned by the entries'
unit(the view name, readable from the chart object), and each view's own data frame is filtered:dictof frames keyed by view name. Give subchartsname=for readable keys; altair's generatedview_...names are used otherwise.df=also accepts such a dict; passing a single frame for a multi-dataset selection raises instead of silently mis-filtering.chart.datais undefined there) now filters that layer's data.alt.selection_point()withoutfields=/encodings=) raise an actionable error on multi-dataset charts: their store holds positional_vgsid_ids, which Vega assigns globally across the whole chart's dataflow (verified in a live browser: clicking the right chart's 6th point of a two-frame hconcat yields_vgsid_: 16), so they can't be mapped back to one view's rows. Declaringfields=/encodings=makes the selection value-based and fully supported.Implementation notes
named_subcharts()walks layer/concat charts to map view name → subchart (duplicate names raise; facet/repeat charts are out of scope and fail loudly rather than mis-filtering).selection_views()reads the hoisted selection param'sviewsto learn which views the selection participates in, falling back to the store'sunits when the param isn't readable (e.g.alt.Chart.from_dictcharts).selectionTestis unchanged — it's just applied per view viafilter_selection_by_unit().Verification
df=dict/scalar rules, duplicate and unresolvable view errors, vegafusion transformer.tests/playwright/altair_accessors).make checkis clean (ruff, pyright, 1157 unit tests passing).