fix: unique view names per concat cell and correct param.views

[mattijn]

After some hours digging.

What’s going wrong (v6.0.0): View and param names in v6.0.0 use content-based hashing for thread-safety. When you concatenate two (or more) identical faceted charts, they get the same content hash, so they all receive the same view name (e.g. view_9f1740faf6e66b88_0). As a result, each param’s views array ends up pointing at that single view instead of one view per concat cell. The compiler then sees duplicate or incorrect bindings and can error or behave wrongly.

What v5 did: v5 used a simple counter for view names (view_5, view_6, …), so each concat cell had a distinct name and each param could correctly list its views (e.g. param.views = ["view_5", "view_6"] for a vconcat of two faceted charts).

Fix direction: Keep content-based hashing, but in the concat case (multiple sibling subcharts), disambiguate by position: assign each subchart a unique view name, e.g. f"{base}_{i}" where i is the concat index. For a FacetChart whose inner spec is a single Chart, put the name on spec.name; for a FacetChart whose inner spec is a LayerChart, put it on the first layer (so param.views can point at those). In _combine_subchart_params, when creating a new param entry (APPEND) we use only this subchart’s view for that entry’s views list (not the param’s existing views), so params that share or reuse view lists don’t get the wrong views. That restores the intended one-view-per-concat-cell behaviour and fixes the error.

Fix #3954

[mattijn]

Willing to review @joelostblom🙃? Its getting a bit complicated with hashing and adding parameters and do proper lifting..

[mattijn]

A bit of explanation using VL-JSON snippets.

Test 1: three faceted charts, three params (p1, p2, p3),

test-name: test_concat_faceted_three_params_unique_views_per_param_issue_3954

v5 (correct, but no hashing) - each concat cell has a distinct view name; each param lists only its cell’s view:

"params": [
  {
    "name": "p1",
    "select": { "type": "point" },
    "views": ["view_1"]
  },
  {
    "name": "p2",
    "select": { "type": "point" },
    "views": ["view_2"]
  },
  {
    "name": "p3",
    "select": { "type": "point" },
    "views": ["view_3"]
  }
]

vconcat cells: spec.name is "view_1", "view_2", "view_3" respectively.

---

v6 before fix (incorrect) - all three cells get the same view name; all three params list that one view:

"params": [
  {
    "name": "p1",
    "select": { "type": "point" },
    "views": ["view_7d5fdb59e52741c4_0"]
  },
  {
    "name": "p2",
    "select": { "type": "point" },
    "views": ["view_7d5fdb59e52741c4_0"]
  },
  {
    "name": "p3",
    "select": { "type": "point" },
    "views": ["view_7d5fdb59e52741c4_0"]
  }
]

vconcat cells: every spec.name is "view_7d5fdb59e52741c4_0".

---

v6 with this PR - one view per cell; each param lists only its cell’s view:

"params": [
  {
    "name": "p1",
    "select": { "type": "point" },
    "views": ["view_7d5fdb59e52741c4_0"]
  },
  {
    "name": "p2",
    "select": { "type": "point" },
    "views": ["view_7d5fdb59e52741c4_1"]
  },
  {
    "name": "p3",
    "select": { "type": "point" },
    "views": ["view_7d5fdb59e52741c4_2"]
  }
]

vconcat cells: spec.name is "view_7d5fdb59e52741c4_0", "view_7d5fdb59e52741c4_1", "view_7d5fdb59e52741c4_2" respectively.

---

Test 2: two faceted charts, one shared param (selection_interval bind=scales),

test-name test_concat_faceted_shared_param_both_views_issue_3954

v5 (correct, but no hashing) - one param listing both cell views; each cell has a distinct layer view name:

"params": [
  {
    "bind": "scales",
    "name": "param_3",
    "select": { "type": "interval" },
    "views": ["view_5", "view_6"]
  }
]

vconcat: first cell spec.layer[0].name = "view_5", second = "view_6".

---

v6 before fix (incorrect) - both cells share the same view name; param lists only that one view:

"params": [
  {
    "bind": "scales",
    "name": "param_f268f798f41c853e",
    "select": { "type": "interval" },
    "views": ["view_9f1740faf6e66b88_0"]
  }
]

vconcat: both cells have spec.layer[0].name = "view_9f1740faf6e66b88_0".

---

v6 with this PR - each cell has a distinct view name; param lists both:

"params": [
  {
    "bind": "scales",
    "name": "param_f268f798f41c853e",
    "select": { "type": "interval" },
    "views": ["view_9f1740faf6e66b88_0", "view_9f1740faf6e66b88_1"]
  }
]

vconcat: first cell spec.layer[0].name = "view_9f1740faf6e66b88_0", second = "view_9f1740faf6e66b88_1".

[joelostblom]

Thanks for putting the time and effort into digging in to this @mattijn! I think your approach makes sense overall and is definitely more proper than the fix we tried earlier. Just a couple of comments / questions.

[mattijn]

There is one thing that I didn't doublecheck, I'm not sure if our test-suite sufficiently covers what happens when facing a chart using repeat + interactivity or a chart with a layered repeat + interactivity, see also vega/vega-lite#6890 (comment), #2849 and #3024.

[joelostblom]

Great job @mattijn! I think the updates fix the previous issues and it is also cleaner to read.

I'm not sure we need a test for repeat... I guess it's always good to be comprehensive but do we expect something different to happen with the parameters when a repeat chart is involved?

[mattijn]

I've double-checked what I liked to do before merging. I was wondering if the following chart still functions, which has some complicated naming of views:

import altair as alt
from altair.datasets import data

source = alt.UrlData(
    data.flights_2k.url,
    format={'parse': {'date': 'date'}}
)

brush = alt.selection_interval(encodings=['x'])

# Define the base chart, with the common parts of the
# background and highlights
base = alt.Chart(source).mark_bar().encode(
    x=alt.X(alt.repeat('column'), type='quantitative', bin=alt.Bin(maxbins=20)),
    y='count()'
).properties(
    width=160,
    height=130
)

# gray background with selection
background = base.encode(
    color=alt.value('#ddd')
).add_params(brush)

# blue highlights on the transformed data
highlight = base.transform_filter(brush)

# layer the two charts & repeat
chart = alt.layer(
    background,
    highlight,
    data=source
).transform_calculate(
    "time",
    "hours(datum.date)"
).repeat(column=["distance", "delay", "time"])
chart

chart.to_dict()

{'config': {'view': {'continuousWidth': 300, 'continuousHeight': 300}},
 'repeat': {'column': ['distance', 'delay', 'time']},
 'spec': {'layer': [{'mark': {'type': 'bar'},
    'encoding': {'color': {'value': '#ddd'},
     'x': {'bin': {'maxbins': 20},
      'field': {'repeat': 'column'},
      'type': 'quantitative'},
     'y': {'aggregate': 'count', 'type': 'quantitative'}},
    'name': 'view_0b48749199e89fbe_0'},
   {'mark': {'type': 'bar'},
    'encoding': {'x': {'bin': {'maxbins': 20},
      'field': {'repeat': 'column'},
      'type': 'quantitative'},
     'y': {'aggregate': 'count', 'type': 'quantitative'}},
    'transform': [{'filter': {'param': 'param_c97a99acfc68b794'}}]}],
  'data': {'url': 'https://cdn.jsdelivr.net/npm/[email protected]/data/flights-2k.json',
   'format': {'parse': {'date': 'date'}}},
  'height': 130,
  'transform': [{'calculate': 'hours(datum.date)', 'as': 'time'}],
  'width': 160},
 'params': [{'name': 'param_c97a99acfc68b794',
   'select': {'type': 'interval', 'encodings': ['x']},
   'views': ['child__column_distance_view_0b48749199e89fbe_0',
    'child__column_delay_view_0b48749199e89fbe_0',
    'child__column_time_view_0b48749199e89fbe_0']}],
 '$schema': 'https://vega.github.io/schema/vega-lite/v6.1.0.json'}

All good from my side as well, merging then:) Thanks for reviewing @joelostblom!