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

chart with repeat and a defined name disables the independency of the repeated channel (eg y-axis) #8446

Closed
mattijn opened this issue Oct 4, 2022 · 7 comments · Fixed by #8733
Labels
Altair Issue that is blocking Altair Bug 🐛

Comments

@mattijn
Copy link
Contributor

mattijn commented Oct 4, 2022

When a name is defined within a layer within a repeat spec as such:

 "repeat": ["temp_max", "precipitation", "wind"],
  "spec": {
    "layer": [
      {
        "name": "FOO",

Than the axis of the chart get confused, see the following screenshot:
image
Open the Chart in the Vega Editor

VL-spec:

{
  "$schema": "https://vega.github.io/schema/vega-lite/v5.json",
  "data": {"url": "data/weather.csv"},
  "repeat": ["temp_max", "precipitation", "wind"],
  "spec": {
    "layer": [
      {
        "name": "FOO",
        "mark": {"type": "circle"},
        "encoding": {
          "x": {"field": "date", "timeUnit": "month"},
          "y": {"field": {"repeat": "repeat"}, "aggregate": "mean"},
          "color": {"field": "location"},
          "size": {"value": 50}
        }
      },
      {
        "mark": {"type": "line"},
        "encoding": {
          "x": {"field": "date", "timeUnit": "month"},
          "y": {"field": {"repeat": "repeat"}, "aggregate": "mean"},
          "color": {"field": "location"}
        }
      }
    ]
  }
}

When removing "name": "FOO" of the spec; the circles will be correctly displayed on the axis again:
image

@mattijn
Copy link
Contributor Author

mattijn commented Oct 4, 2022

Hopefully the cause of #8348 🙏

@mattijn mattijn changed the title chart with repeat and a defined name disables the independency of the y-axis chart with repeat and a defined name disables the independency repeated channel (eg y-axis) Oct 4, 2022
@mattijn mattijn changed the title chart with repeat and a defined name disables the independency repeated channel (eg y-axis) chart with repeat and a defined name disables the independency of the repeated channel (eg y-axis) Oct 4, 2022
@ChristopherDavisUCI
Copy link
Contributor

Hi @mattijn, my best guess is that for your example, in the code here childSpec is a layer spec in which one of the layers is named. If the full layer spec itself had a name, I think these lines would correctly update that name. But in your case, it's one of the constituent layers that has a name, and I think that name is getting copied without any special prefix, so the same name is showing up three times in the final result. Maybe something is going wrong in the creation of child here:

const child = this.map(childSpec, {...params, repeater: childRepeater, repeaterPrefix: childName});

There is already some special logic for dealing with layers here; see especially this line. But if I understand right, that code only gets called if we have something like "repeat": {"layer": ...}, unlike your case, where the spec is a layer chart.

Does that give you any ideas?

@ChristopherDavisUCI
Copy link
Contributor

Hi @mattijn, here is a first attempt at addressing this issue of the same name showing up multiple times: 0ab291a

(It's the same as I posted on #8348.)

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

How critical is this issue? It seems weird to name a chart inside a repeat since the name will not be predictable anyway. Can you just remove the name from the chart?

@mattijn
Copy link
Contributor Author

mattijn commented Feb 20, 2023

Its eventually to get this spec working, from here: #8348 (comment):

Click to open Chart in Vega-Editor

The spec renders, but once you start interacting, e.g. start with creating an interval at the last chart, x-axis time (binned), and then delay (binned) and then distance (binned), there are conflicts.

Even though this type of chart is maybe not very common in practice, it is one of the selling-charts of vega-lite.

The layer name is predictable as it is defined in Altair code.

See also quote from 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...

@domoritz
Copy link
Member

 > The layer name is predictable as it is defined in Altair code.

What do you mean by this? I don't think were (Vega-Lite) make guarantees about generated names being consistent across versions.

@mattijn
Copy link
Contributor Author

mattijn commented Feb 20, 2023

Our current approach of providing interactivity using params for compound charts is based on this #8230 (comment):

VariableParameters are only parsed and assembled when they appear at the top-level.

SelectionParameters necessarily need some view context for direct manipulation. If they're defined within views, that happens implicitly but, if they're defined at the top-level, they need a views property specified which can either be a complete name of a view or a partial path through the view tree.

So, I think a consistent approach Altair can take is to always hoist parameter definitions to the top-level. If the parameter is a selection, then add a corresponding views property and name the constituent view.

Example: Open the Chart in the Vega Editor

Using this approach we almost got all examples working. Except the layered repeat charts + interactivity. But just before the layered repeat charts + interactivity, are the repeat charts + interactivity. This we still got working by

  1. Defining a name in the spec:
  "spec": {
    "name": "view_1",
  1. Checking what is being repeated.
  "repeat": {
    "column": ["sepalLength", "sepalWidth"],
    "row": ["petalLength", "petalWidth"]
  },
  1. Construct the views property in the params:
  "params": [
    {
      "name": "param_1",
      "select": {"type": "interval", "encodings": ["x", "y"]},
      "bind": "scales",
      "views": [
        "view_1child__row_petalLengthcolumn_sepalLength",
        "view_1child__row_petalLengthcolumn_sepalWidth",
        "view_1child__row_petalWidthcolumn_sepalLength",
        "view_1child__row_petalWidthcolumn_sepalWidth"
      ]
    }
  ]

So here it is <view-name>child__<repeat-row>_<data-field><repeat-column>_<data-field>

I took the code snippets from the following example specification that we used during development: Open the Chart in the Vega Editor, see related vega/altair#2684 (comment).

Regarding the layered repeat charts + interactivity, I don't think we implemented this part yet on the Altair side, see vega/altair#2849, since it is also still halted here.

All above is implemented in the following section of the Altair codebase.
https://github.com/altair-viz/altair/blob/master/altair/vegalite/v5/api.py#L2887-L3062


Addendum: hopefully it won't give noise, but this is my notebook with all type of Altair based compound charts that we have tested our developments on: colab notebook

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

Successfully merging a pull request may close this issue.

3 participants