vega-lite icon indicating copy to clipboard operation
vega-lite copied to clipboard

repeated chart cannot handle `name` property in combination with cross-view parameter selection

Open mattijn opened this issue 2 years ago • 1 comments

This issue is linked to this comment: https://github.com/vega/vega-lite/issues/8230#issuecomment-1201358652. Example is based on a repeated chart such as this example: https://vega.github.io/vega-lite/examples/interactive_layered_crossfilter.html

Upon manually defining a "name":"CHART" property for the view defining the param + filter the units-field in the brush_store is fixed to the name "CHART", as can be seen in the data-viewer below. No matter which view is being brushed. This makes the brush selection corrupt cross-view.

image

Spec that errors: Open the Chart in the Vega Editor


Comparison to not using a manual defined name property:

When I make a brush selection the first view (most left), I observe that the unit field is filled with "child__column_distance_layer_0" and when changing the brush selection to the middle-plot the unit field changes to "child__column_delay_layer_0" and the last view has an unit in the brush store defined as "child__column_time_layer_0".

image

I somehow hoped I could use these programmatically defined units as different views in a top-level defined params object as such:

"params": [
  {
    "name": "brush",
    "select": {"type": "interval", "encodings": ["x"]},
    "views": ["child__column_distance_layer_0", "child__column_delay_layer_0", "child__column_time_layer_0"]
  }
]

but to no avail ([Error] Unrecognized signal name: "brush")..

mattijn avatar Aug 08 '22 21:08 mattijn

In this comment by @arvind https://github.com/vega/vega-lite/issues/8230#issuecomment-1179213186 is mentioned:

[a views property specified] can either be a complete name of a view or a partial path through the view tree.

I was wondering if this partial path through the view tree can be of usage here?

If there is some documentation available on these partial path and view tree or if someone can give some insights than I can investigate by myself a bit more as well.

mattijn avatar Aug 18 '22 20:08 mattijn

Following the described logic from here, https://github.com/vega/vega-lite/issues/8452#issuecomment-1279995980, I think I followed the partial path through the view tree correctly, but I was not able to succeed. Reading through this issue, I've tried it before, but without a spec that can be directly opened in the editor, now one can: Open the Chart in the Vega Editor. The error still is:

Unrecognized signal name: "brush"

mattijn avatar Oct 16 '22 18:10 mattijn

Hi @mattijn,

There are two things at play here that are preventing the spec from working correctly:

(1) Neither of the layers have a name property, which does not allow them to be addressed as views in top-level selection parameters.

(2) To address a nested view, views needs to be an array of names rather than concatenating the names together.

Here's a working spec

(There technically is a bug that causes duplicate names to be added if the top-level view is named (as is the case here since the top-level view is a repeated view). This bug should be fixed once #8486 is merged. But, as far as I can tell, this bug doesn't prevent this spec from working.)

Hope this helps!

arvind avatar Oct 17 '22 01:10 arvind

Actually, I'm reopening this because the "working spec" I linked to above is showing the behavior you screenshotted in the OP. It seems like multiple brushes appear even though there should only be one. I think I now better understand the issue you're describing.

Will keep investigating — it seems like at the normalized stage, the layered views don't seem to get assigned a name? That seems to be happening at some later stage, which is why child__column_distance_layer_0 show up in the selection stores at run time...

arvind avatar Oct 17 '22 01:10 arvind

Thanks, as always @arvind! Your 2nd listed point is eye-opening:

(2) To address a nested view, views needs to be an array of names rather than concatenating the names together.

I tried lots of things, but this not. Sometimes I feel like being a VL-philologist;).

Regardless the outstanding behavior, which is almost as it should be (stay positive), it's good to know that we can create the right specification with top-declared parameters using this logic.

mattijn avatar Oct 17 '22 09:10 mattijn

Hi @mattijn, I made a first attempt at fixing this problem here: https://github.com/vega/vega-lite/commit/0ab291a413c58728212c2a9570afbb6b99185454

The resulting extended Vega-Lite for your first chart above is this: Open the Chart in the Vega Editor. Does that seem to be working as expected?

ChristopherDavisUCI avatar Jan 22 '23 01:01 ChristopherDavisUCI

Yes, your example works correctly, but that is because the params are localized and not defined top-level like we do in Altair isn't it?

How is this example related to the change you introduced that hopefully fix the issue (would be great btw!)?

Probably not related to this issue: I also noticed a top-level "align": "all" key-val in the spec. I can't recall seeing that before. Did you deliberately add this?

mattijn avatar Jan 22 '23 08:01 mattijn

Hi @mattijn, I think these changes you mention always(?) happen in the transition to the "extended Vega-Lite spec" (visible at the bottom of the Vega editor). For example, I think they also happen in your example at the top. The only change I made was to add a prefix to certain names, replacing spec.name by params.repeaterPrefix + spec.name when both spec.name and params.repeaterPrefix are defined.

What I linked in https://github.com/vega/vega-lite/issues/8348#issuecomment-1399378752 was the extended Vega-Lite spec, but the original Vega-Lite spec that I entered was the same as yours.

I might make a PR with this change just to see what the feedback is. I don't think this exact change to the name is what they will want, but if we're lucky maybe the location of the change and/or the logic is correct.

ChristopherDavisUCI avatar Jan 22 '23 12:01 ChristopherDavisUCI

I was not aware that the extended Vega-Lite spec is doing some translation of the top-level parameters to nested parameters. I don't think the used spec in OP is valid to the current issue we are seeing.

Does your fix also works for this spec: Open the Chart in the Vega Editor from this comment of @arvind? Because that spec is actually defining top-level params as what we are intend to do in Altair as well.

Another thing I notice from @arvind spec is that it is using these concatenation of names as arrays with defined layer:

"views": [
  ["child__column_distance", "layer_0"],
  ["child__column_delay", "layer_0"],
  ["child__column_time", "layer_0"]
]

But If I look to what type of Vega-lite we generate from the following Altair spec than I don't see this is happening:

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

brush = alt.selection(type='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"])

In my case it is just:

"views": ["view_9"]

Did we ever implemented this? Developments are going so quick last few weeks, I can't remember it anymore.

mattijn avatar Jan 22 '23 15:01 mattijn

Thanks @mattijn! No, that spec you linked is not working for me either, but it gives a different error, Unrecognized signal name: "brush", so maybe that is a sign of progress. (The error shows up for me right away, as opposed to the current version of Vega-Lite, where the error doesn't show up until you attempt to use the parameter.)

My memory is also a little fuzzy about the special names, but I remember we did implement at least some of it here: https://github.com/altair-viz/altair/pull/2684/commits/f5473234a43b054e778541665544086895d65d9f So far I haven't succeeded in finding an example where this code actually gets used, but I will keep looking!

ChristopherDavisUCI avatar Jan 22 '23 16:01 ChristopherDavisUCI

Isn't this the example where that code should be triggered?

mattijn avatar Jan 22 '23 16:01 mattijn

Maybe?! But my inclination is always to give the benefit of the doubt to my past self, not to my current self who has looked at it for 2 minutes 😝

ChristopherDavisUCI avatar Jan 22 '23 16:01 ChristopherDavisUCI

Yes, my current self is also much more confused than the moment we implemented that part😀

mattijn avatar Jan 22 '23 16:01 mattijn

I'll try to look over this stuff carefully in the next few days.

ChristopherDavisUCI avatar Jan 22 '23 16:01 ChristopherDavisUCI

@ChristopherDavisUCI I opened https://github.com/altair-viz/altair/issues/2849 to track the Altair issue described above. One more thing, you mention the following:

The error shows up for me right away, as opposed to the current version of Vega-Lite, where the error doesn't show up until you attempt to use the parameter.

But the current version does not give an error, but a warning:

[Warning] Infinite extent for field "__count": [Infinity, -Infinity]

So the interaction is still doing something, but just something inaccurate.

mattijn avatar Jan 23 '23 09:01 mattijn

@mattijn I took a closer look at Arvind's example you linked in https://github.com/vega/vega-lite/issues/8348#issuecomment-1399528866. In that example, we have

"views": [
        ["child__column_distance", "layer_0"],
        ["child__column_delay", "layer_0"],
        ["child__column_time", "layer_0"]
      ]

In the resulting concat chart, three views with the name "layer_0" appear. My strategy in #8663 is to give those views different names. When I try Arvind's spec (with PR #8663) with the following change it works:

"views": [
        "child__column_distancelayer_0",
        "child__column_delaylayer_0",
        "child__column_timelayer_0"
      ]

Any first impressions on that?

It doesn't seem worthwhile making a corresponding fix on the Altair side, because even if this is a good strategy, I doubt the exact naming I did (just concatenating the name and the prefix) is what should be done on the Vega-Lite side.

ChristopherDavisUCI avatar Jan 26 '23 13:01 ChristopherDavisUCI

I just noticed your comment @ChristopherDavisUCI, but

["child__column_distance", "layer_0"],

Isn't equal to

"child__column_distancelayer_0",

Am I right in understanding your comment is that the currently defined altair-views are resolving, or at least should resolve into the above in the extended vega lite specification on the VL-side?

mattijn avatar Feb 05 '23 17:02 mattijn

Thanks for checking it out @mattijn. Think of my comment above as having nothing to do with Altair at this point. (My idea is that once the issue from Arvind's chart is resolved on the Vega-Lite side, we can hopefully work something out to match it on the Altair side.) In #8663 I give unique names to the various views and that seemed to get the chart working. (I didn't put much thought into what exact names to use, figuring I didn't know what conventions, like how many underscores, should be used. All I did was prepend the repeaterPrefix onto the beginning of the current name.)

Does that make sense? I believe as things currently are defined in Vega-Lite, in the extended specs, some views can have the same name (and are only distinguished by having a different path through the view tree).

ChristopherDavisUCI avatar Feb 05 '23 17:02 ChristopherDavisUCI

Ok! More clear now. I'll subscribe to #8663 to see how this lands in VL👍

mattijn avatar Feb 05 '23 18:02 mattijn

If I'm not misunderstanding things, than this issue is fixed by #8733, see https://github.com/vega/vega-lite/pull/8733#issuecomment-1451337875. Please re-open if I'm wrong on this.

mattijn avatar Mar 22 '23 11:03 mattijn