duplicate parameter definition error in layered charts (because of "point": true)

[mattijn]

Original observed in this Altair issue comment: vega/altair#2528 (comment).
Open the Chart in the Vega Editor

{
  "params": [
    {
      "name": "highlight",
      "select": {"type": "point", "clear": "mouseout", "on": "mouseover"}
    }
  ],
  "vconcat": [
    {
      "height":100,
      "mark": {"type": "line", "point": true},
      "encoding": {
        "x": {"field": "date", "timeUnit": "year", "type": "nominal"},
        "y": {"aggregate": "mean", "field": "price", "type": "quantitative"}
      }
    },
    {
      "mark": {"type": "rect"},
      "encoding": {
        "fill": {"aggregate": "mean", "field": "price", "type": "quantitative"},
        "stroke": {
          "condition": {"value": "black", "param": "highlight", "empty": false},
          "value": null
        },
        "x": {"field": "date", "timeUnit": "year", "type": "temporal"}
      }
    }
  ],
  "data": {"url": "data/stocks.csv"},
  "$schema": "https://vega.github.io/schema/vega-lite/v5.2.0.json"
}

gives

When setting -upon first sight unrelated- "point": true to "point": false it will work:

Defining the parameter on the view, it will also work with "point": true.

{
  "vconcat": [
    {
      "height": 100,
      "mark": {"type": "line", "point": true},
      "encoding": {
        "x": {"field": "date", "timeUnit": "year", "type": "nominal"},
        "y": {"aggregate": "mean", "field": "price", "type": "quantitative"}
      },
      "params": [
        {
          "name": "highlight",
          "select": {"type": "point", "clear": "mouseout", "on": "mouseover"}
        }
      ]
    },
    {
      "mark": {"type": "rect"},
      "encoding": {
        "fill": {"aggregate": "mean", "field": "price", "type": "quantitative"},
        "stroke": {
          "condition": {"value": "black", "param": "highlight", "empty": false},
          "value": null
        },
        "x": {"field": "date", "timeUnit": "year", "type": "temporal"}
      }
    }
  ],
  "data": {"url": "data/stocks.csv"},
  "$schema": "https://vega.github.io/schema/vega-lite/v5.2.0.json"
}

[mattijn]

This spec is another example: Open the Chart in the Vega Editor:

{
  "data": {
    "values": [
      {"a": 1, "b": 1, "c": 1, "d": 1},
      {"a": 2, "b": 2, "c": 1, "d": 2},
      {"a": 3, "b": 1, "c": 2, "d": 3},
      {"a": 4, "b": 2, "c": 2, "d": 4}
    ]
  },
  "facet": {"field": "c", "type": "nominal"},
  "params": [
    {
      "name": "selected",
      "select": {"type": "point", "fields": ["d"]},
      "bind": "legend",
      "views": ["view_1"]
    }
  ],
  "spec": {
    "name": "view_1",
    "mark": {"type": "line", "point": true},
    "encoding": {
      "color": {"field": "d", "type": "nominal"},
      "x": {"field": "a", "type": "quantitative"},
      "y": {"field": "b", "type": "quantitative"},
      "opacity": {"condition": {"value": 1, "param": "selected"}, "value": 0.25}
    }
  },
  "$schema": "https://vega.github.io/schema/vega-lite/v5.2.0.json"
}

When changing "point": true to "point": false the error disappear (but the chart won't appear as requested).
Manually defining both the circle and line mark will work though.

{
  "data": {
    "values": [
      {"a": 1, "b": 1, "c": 1, "d": 1},
      {"a": 2, "b": 2, "c": 1, "d": 2},
      {"a": 3, "b": 1, "c": 2, "d": 3},
      {"a": 4, "b": 2, "c": 2, "d": 4}
    ]
  },
  "facet": {"field": "c", "type": "nominal"},
  "spec": {
    "layer": [
      {
        "mark": "line",
        "encoding": {
          "color": {"field": "d", "type": "nominal"},
          "opacity": {
            "condition": {"value": 1, "param": "selected"},
            "value": 0.25
          },
          "x": {"field": "a", "type": "quantitative"},
          "y": {"field": "b", "type": "quantitative"}
        },
        "name": "view_1"
      },
      {
        "mark": "circle",
        "encoding": {
          "color": {"field": "d", "type": "nominal"},
          "opacity": {
            "condition": {"value": 1, "param": "selected"},
            "value": 0.25
          },
          "x": {"field": "a", "type": "quantitative"},
          "y": {"field": "b", "type": "quantitative"}
        }
      }
    ]
  },
  "params": [
    {
      "name": "selected",
      "select": {"type": "point", "fields": ["d"]},
      "bind": "legend",
      "views": ["view_1"]
    }
  ],
  "$schema": "https://vega.github.io/schema/vega-lite/v5.2.0.json"
}

Open the Chart in the Vega Editor

I assume the point=True will duplicate the spec under the hood into a layered chart, but it also duplicates the "name": "view_1" for both layers and that causes this error, since I define "name": "view_1" on only a single view in the manual layered chart.

[ChristopherDavisUCI]

Hi @mattijn, I was looking at a variant of your example #7854 (comment) above. Here is the version I was looking at: Open the Chart in the Vega Editor. My version is one step simpler than yours because mine has "point": false. (I also slightly changed the data so that there are lines present.)

If you open my version in the Vega editor and click on "Extended Vega-Lite Spec" at the bottom and then "Edit Extended Vega-Lite Spec", you'll see that the parameter has been moved from the top level to inside the spec object. Also warnings appear in the logs because of the presence of the "views" property in this non-top-level parameter.

I can't quite follow the code, but based on this comment, my guess is that it might be related to the normalizeGenericSpec function.

Does that give you any ideas?

[mattijn]

That might be another issue.
I think the problem here is that the "point": true will duplicate the spec under the hood somewhere into a layered version of a line mark encoding and a circle mark encoding, where the latter is a duplicate of the mark line with only the the mark line to circle replaced. But since there is also a name definition ("name": "view_1"), it also duplicates that name and that is problematic since it is not allowed to have two identical name definitions in different chart objects in a layered chart.
If in the duplication process of creating circle mark chart object the key name is popped then the issue will resolve. I think.

If I modify your example into a manual layered version of a line and mark, Open the Chart in the Vega Editor, it starts with the same error as with "point": true (you can check and search for selected_tuple in the 'Compiled Vega'), but if you remove line 35 from the Vega-Lite specification (and thus removing the name of the second chart in the layered spec) than it works.

Can we find the location in the code where the "point": true is start being processed?

[ChristopherDavisUCI]

Thank you! I haven't completely processed what you wrote @mattijn, but I do think I tracked down where the "point": true starts to be processed. This is my best guess:

vega-lite/src/normalize/pathoverlay.ts

Lines 162 to 174 in 2ddf383

	if (pointOverlay) {
	layer.push({
	...(projection ? {projection} : {}),
	mark: {
	type: 'point',
	opacity: 1,
	filled: true,
	...pick(markDef, ['clip', 'tooltip']),
	...pointOverlay
	},
	encoding: overlayEncoding
	});
	}

[mattijn]

Will changing this sentence (on line 170):

...pointOverlay 

into

...omit(pointOverlay, ['name'])

be sufficient?

It seems that omit is a custom function defined in utils which delete keys from a copied dictionary where the delete command in typescript will not fail if the key is not detected.

[mattijn]

Nope, that is one level too deep. I think my suggestion will turn this:

"name": "foo"
"mark": {"type":"point", "filled": true, "name": "bah"},

into:

"name": "foo"
"mark": {"type":"point", "filled": true},

Where we like to "omit" "name": "foo". Maybe somewhere around line 164?

[ChristopherDavisUCI]

Hi @mattijn, do you know what's the best way to test these kind of code changes? Is it opening a linked Vega Editor? Or is there a more natural choice like running the code in VS Code and using debugging? (All the (limited) JavaScript I've played with before has been website-focused, so I always tested it by using a web browser.)

[mattijn]

There is a contributing page in the repo. It mentions VS Code as preferable IDE: https://github.com/vega/vega-lite/blob/next/CONTRIBUTING.md#suggested-programming-environment.

But I've no much experience in this either.

[ChristopherDavisUCI]

Thanks @mattijn. I'm just experimenting at this stage, but can you take a look at https://github.com/ChristopherDavisUCI/vega-lite/commit/1ced888950ce5580266fc1fc4f35a902238bd449 and see if you think it helps the error from #7854 (comment) ?

It does not seem to help your initial vconcat example. Edit. It doesn't fix your vconcat example at the top of this issue, but the Altair chart that originally produced that spec has already been fixed (thanks to the use of named views), so from the Altair side, I don't think we need to worry about your vconcat example. Here is the json produced by Altair for your example: Open the Chart in the Vega Editor

[binste]

@ChristopherDavisUCI Nice catch, I missed the implementation of named views, this is great! For reference, produces

import altair as alt
from vega_datasets import data

stocks = data.stocks.url
highlight = alt.selection_point(name="highlight", on="mouseover", clear="mouseout")

line = (
    alt.Chart(height=100)
    .mark_line(point=True)
    .encode(x=alt.X("date:N", timeUnit="year"), y=alt.Y("mean(price):Q"))
)

rect = (
    alt.Chart()
    .mark_rect()
    .encode(
        x=alt.X("date:T", timeUnit="year"),
        fill=alt.Y("mean(price):Q"),
        stroke=alt.condition(
            highlight, alt.value("black"), alt.value(None), empty=False
        ),
    )
    .add_params(highlight)
)

alt.vconcat(line, rect, data=stocks)

{
  "$schema": "https://vega.github.io/schema/vega-lite/v5.2.0.json",
  "data": {
    "url": "https://cdn.jsdelivr.net/npm/vega-datasets@v1.29.0/data/stocks.csv"
  },
  "params": [
    {
      "name": "highlight",
      "select": {"clear": "mouseout", "on": "mouseover", "type": "point"},
      "views": ["view_3"]
    }
  ],
  "vconcat": [
    {
      "encoding": {
        "x": {"field": "date", "timeUnit": "year", "type": "nominal"},
        "y": {"aggregate": "mean", "field": "price", "type": "quantitative"}
      },
      "height": 100,
      "mark": {"point": true, "type": "line"}
    },
    {
      "encoding": {
        "fill": {"aggregate": "mean", "field": "price", "type": "quantitative"},
        "stroke": {
          "condition": {"empty": false, "param": "highlight", "value": "black"},
          "value": null
        },
        "x": {"field": "date", "timeUnit": "year", "type": "temporal"}
      },
      "mark": "rect",
      "name": "view_3"
    }
  ]
}

which is valid. Only difference is the naming of "view_3" and that the parameter references it. Does this mean that this issue is no longer blocking the Altair 5 release or is there still a common pattern in Altair that is affected?

[ChristopherDavisUCI]

Thanks for checking this out, @binste! You're right that @mattijn's original chart now works in Altair, but I believe this is an example where an update is still needed:

import altair as alt
import pandas as pd

sel = alt.selection_point(fields=["c"])

df = pd.DataFrame({
    "a": [1,2,3,4],
    "b": [1,2,1,2],
    "c": [1,1,2,2]
})

alt.Chart(df).mark_line(point=True).encode(
    x="a",
    y="b",
    color="c:N",
).facet(
    "c:N"
).add_params(sel)

Like in the other examples, changing to point=False will remove the problem.

I think my code here fixes this particular issue, but I'm not sure if it's a reasonable approach or if it causes other problems.

[mattijn]

Yes there is still a common pattern affected. The second comment is still problematic. It was raised as well in vega/altair#2713.
I'm somehow surprised your code fix this issue @ChristopherDavisUCI. It is the right thing to do, but before the name key was not yet injected there, so it was done elsewhere. Maybe there is a check if there is a name defined it does not do this injection.
Does it pass the test bank of Vega-Lite? Otherwise turn it into a PR.

[ChristopherDavisUCI]

Thanks @mattijn! My understanding is that this line is where the name previously got injected, and so what I tried to do is to remove the "name" property from the encoding at an earlier stage, and then only include it in one spot. Does that make sense?

I will try running the test suite now.

[mattijn]

You remove the name from the encoding? In my understanding there is no name key within encoding.

I think what you do is you import the name and define it on the first item in the layered specification, just like we make sure on the Altair side to have name only be defined on the first item in a manual layered specification. So that make sense.

But how you knew you could define name as const around here?
https://github.com/ChristopherDavisUCI/vega-lite/commit/1ced888950ce5580266fc1fc4f35a902238bd449#diff-9d97dcc41d7b274e7a6ad719846874fdd3ebb66ecd70b82dc1ee228e4a598425R104

Which name is this?

[ChristopherDavisUCI]

You're right, I meant that I remove name from the spec (not from the encoding). This is where that happens: https://github.com/ChristopherDavisUCI/vega-lite/blob/1ced888950ce5580266fc1fc4f35a902238bd449/src/normalize/pathoverlay.ts#L104

The main point is that the name property doesn't get duplicated. Does that make more sense? (If spec does not have a name property, than my JavaScript understanding is that this name will just be undefined.)

Does that make more sense?

[mattijn]

Yes it does! Thanks for explaining. I notice there is also an lineOverlay next to pointOverlay. I don't think we had that combination tested yet on the Altair side. So maybe that is another potential-future-issue solved by this change.

[ChristopherDavisUCI]

Great, thanks @mattijn! I tried making a PR here: #8662. Even if it turns out this isn't the right solution, hopefully it can get us on the right track.

[mattijn]

Fixed by #8662