Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repeat chart with top level defined brush selection corrupts cross-view interaction #8348

Closed
mattijn opened this issue Aug 8, 2022 · 20 comments
Labels
Altair Issue that is blocking Altair Bug 🐛

Comments

@mattijn
Copy link
Contributor

mattijn commented Aug 8, 2022

This issue is linked to this comment: #8230 (comment).
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
Copy link
Contributor Author

mattijn commented Aug 18, 2022

In this comment by @arvind #8230 (comment) 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
Copy link
Contributor Author

mattijn commented Oct 16, 2022

Following the described logic from here, #8452 (comment), 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"

@arvind
Copy link
Member

arvind commented Oct 17, 2022

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 arvind closed this as completed Oct 17, 2022
@arvind
Copy link
Member

arvind commented Oct 17, 2022

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 arvind reopened this Oct 17, 2022
@mattijn
Copy link
Contributor Author

mattijn commented Oct 17, 2022

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 mattijn changed the title repeated chart cannot handle name property in combination with cross-view parameter selection repeat chart with top level defined brush selection corrupts cross-view interaction Oct 29, 2022
@ChristopherDavisUCI
Copy link
Contributor

Hi @mattijn, I made a first attempt at fixing this problem here: 0ab291a

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?

@mattijn
Copy link
Contributor Author

mattijn commented Jan 22, 2023

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?

@ChristopherDavisUCI
Copy link
Contributor

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 #8348 (comment) 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.

@mattijn
Copy link
Contributor Author

mattijn commented Jan 22, 2023

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.

@ChristopherDavisUCI
Copy link
Contributor

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:
vega/altair@f547323
So far I haven't succeeded in finding an example where this code actually gets used, but I will keep looking!

@mattijn
Copy link
Contributor Author

mattijn commented Jan 22, 2023

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

@ChristopherDavisUCI
Copy link
Contributor

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 😝

@mattijn
Copy link
Contributor Author

mattijn commented Jan 22, 2023

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

@ChristopherDavisUCI
Copy link
Contributor

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

@mattijn
Copy link
Contributor Author

mattijn commented Jan 23, 2023

@ChristopherDavisUCI I opened vega/altair#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.

@ChristopherDavisUCI
Copy link
Contributor

@mattijn I took a closer look at Arvind's example you linked in #8348 (comment). 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.

@domoritz domoritz added the Altair Issue that is blocking Altair label Feb 3, 2023
@mattijn
Copy link
Contributor Author

mattijn commented Feb 5, 2023

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?

@ChristopherDavisUCI
Copy link
Contributor

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).

@mattijn
Copy link
Contributor Author

mattijn commented Feb 5, 2023

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

@mattijn
Copy link
Contributor Author

mattijn commented Mar 22, 2023

If I'm not misunderstanding things, than this issue is fixed by #8733, see #8733 (comment).
Please re-open if I'm wrong on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Altair Issue that is blocking Altair Bug 🐛
Projects
None yet
Development

No branches or pull requests

4 participants