vega / altair

Declarative statistical visualization library for Python
https://altair-viz.github.io/
BSD 3-Clause "New" or "Revised" License
9.26k stars 793 forks source link

parameters added to a layer should be serialised outside the scope of the layer #2597

Closed mattijn closed 1 year ago

mattijn commented 2 years ago

A single chart object works with parameters

import altair as alt

range0 = alt.binding_range(min=-180, max=180, step=5)
rotate0 = alt.parameter(value=90, bind=range0, name='rotate0')

range1 = alt.binding_range(min=-180, max=180, step=5)
rotate1 = alt.parameter(value=-5, bind=range1, name='rotate1')

graticule = alt.Chart(alt.graticule(step=[15, 15])).mark_geoshape(
    stroke='black'
)

graticule.project(
    'orthographic',
    rotate=alt.ExprRef(f'[{rotate0.name}, {rotate1.name}, 0]')
).add_parameter(rotate0, rotate1)
image

Now I like to combine the graticules with a sphere, so I layer them. First without parameters (this still works):

import alt as alt

sphere = alt.Chart(alt.sphere()).mark_geoshape(
    fill='aliceblue', stroke='black'
)

graticule = alt.Chart(alt.graticule(step=[15, 15])).mark_geoshape(
    stroke='black'
)

alt.layer(sphere, graticule).project(
    'orthographic',
    rotate=[90, -5, 0]
)
image

But it breaks if I add the parameters on the layered chart:

import altair as alt

range0 = alt.binding_range(min=-180, max=180, step=5)
rotate0 = alt.parameter(value=90, bind=range0, name='rotate0')

range1 = alt.binding_range(min=-180, max=180, step=5)
rotate1 = alt.parameter(value=-5, bind=range1, name='rotate1')

sphere = alt.Chart(alt.sphere()).mark_geoshape(
    fill='aliceblue', stroke='black'
)

graticule = alt.Chart(alt.graticule(step=[15, 15])).mark_geoshape(
    stroke='black'
)

alt.layer(sphere, graticule).project(
    'orthographic',
    rotate=alt.ExprRef(f'[{rotate0.name}, {rotate1.name}, 0]')
).add_parameter(rotate0, rotate1)
Javascript Error: Unrecognized signal name: "rotate0"
This usually means there's a typo in your chart specification. See the javascript console for the full traceback.

It is because the params are added to the first feature in the layer instead of being serialised outside the scope of the layer. Here I would expect it to create a global params definition. I think the later is what I would expect if I add parameters to alt.layer(), so I don't feel that https://github.com/vega/vega-lite/issues/7812 applies here.

I can manually override, what I think is wrongly behaviour, with the following:

comb = alt.layer(sphere,graticule).project(
    'orthographic',
    rotate=[90, -5, 0]
).add_parameter(rotate0, rotate1)

chart_vl = comb.to_dict()
chart_vl['params'] =  chart_vl['layer'][0].pop('params')
chart_vl['projection']['rotate'] = {'expr':'[rotate0, rotate1, 0]'}
alt.Chart().from_dict(chart_vl)
image

Side note: the .project() is also defined on the alt.layer(), but is serialised correctly outside the scope of the layer (here globally)

cc: @ChristopherDavisUCI, what you think? Do we have control over this in Altair, or it happens in the VL-serialization?

ChristopherDavisUCI commented 2 years ago

Thanks for bringing this up @mattijn.

I'm pretty confident we can control this on the Altair end, so I think it's just a matter of having a clear idea of what we want. Since the beginning of this update, getting parameters to work correctly with multi-view charts has been one of the biggest hurdles.

I might not have a chance to look at this until the weekend.

ChristopherDavisUCI commented 2 years ago

Thanks again for bringing this up @mattijn!

I'm hopeful that whatever we want to do will be easy to accomplish. I think the difficult part is deciding what we're supposed to do.

In a lot of cases I just copied the old Altair code. For example, here is some Altair v3 code for the add_selection method of a ConcatChart:

https://github.com/altair-viz/altair/blob/0217b2e73703d3b7b529b73b4dec9c17e7fb09bb/altair/vegalite/v3/api.py#L1907-L1913

Here is some Altair v3 code for the add_selection method of a LayerChart (ignore the docstring which I think is incorrect in this case):

https://github.com/altair-viz/altair/blob/0217b2e73703d3b7b529b73b4dec9c17e7fb09bb/altair/vegalite/v3/api.py#L2053-L2059

@mattijn, do you agree that my copying this old LayerChart code is what caused your issue?

To me it's not obvious what to do. I think the most elegant solution would be to get rid of all this "add to all subcharts" functionality, but that would break a lot of old examples. One idea is we could keep that functionality for the deprecated add_selection and get rid of it for the new add_parameter.

In case we've all forgotten they exist, here are some possibly(??) related issues: https://github.com/vega/vega-lite/issues/7812 https://github.com/vega/vega-lite/issues/7854

Anyway, I'm hopeful the coding can be done fairly easily. If you give me precise instructions for what should happen when add_parameter or add_selection is called on a LayerChart or a ConcatChart or a ..., then I think it will be no problem to accomplish that.

mattijn commented 2 years ago

I agree that just copying the current selection implementation will not work with parameters. And there is no easy way out.. it's for example not possible to add all parameters to the top-level.

Eg: Last week I had this example:

import altair as alt
from vega_datasets import data
import geopandas as gpd

# load data
gdf_quakies = gpd.read_file(data.earthquakes.url, driver='GeoJSON')
gdf_world = gpd.read_file(data.world_110m.url, driver='TopoJSON')

# define parameters
range0 = alt.binding_range(min=-180, max=180, step=5)
rotate0 = alt.parameter(value=120, bind=range0, name='rotate0')
hover = alt.selection_point(on='mouseover', clear='mouseout')

# world disk
sphere = alt.Chart(alt.sphere()).mark_geoshape(
    fill='aliceblue',
    stroke='black',
    strokeWidth=1.5
)

# countries as shapes
world = alt.Chart(gdf_world).mark_geoshape(
    fill='mintcream',
    stroke='black',
    strokeWidth=0.35
)

# earthquakes as circles with fill for depth and size for magnitude
# the hover param is added on the mar_circle only
quakes = alt.Chart(gdf_quakies).mark_circle(
    opacity=0.35,
    tooltip=True,
    stroke='black'
).transform_calculate(
    lon="datum.geometry.coordinates[0]",
    lat="datum.geometry.coordinates[1]",
    depth="datum.geometry.coordinates[2]"
).transform_filter('''
    (rotate0 * -1) - 90 < datum.lon && datum.lon < (rotate0 * -1) + 90
    '''
).encode(
    longitude='lon:Q',
    latitude='lat:Q',
    strokeWidth=alt.condition(hover, alt.value(1, empty=False), alt.value(0)),
    size=alt.Size('mag:Q', scale=alt.Scale(type='pow', range=[1,1000], domain=[0,6], exponent=4)),
    fill=alt.Fill('depth:Q', scale=alt.Scale(scheme='lightorange', domain=[0,400]))
).add_parameter(hover)

# define projection and add the rotation param for all layers
comb = alt.layer(sphere, world, quakes).project(
    'orthographic',
    rotate=[90, 0, 0]
).add_parameter(rotate0)

# temporary changing params to top-level
# and defining the rotate reference expression on compiled VL directly
chart_vl = comb.to_dict()
chart_vl['params'] =  chart_vl['layer'][0].pop('params')
chart_vl['projection']['rotate'] = {'expr':'[rotate0, 0, 0]'}
alt.Chart().from_dict(chart_vl)

Here, I had to add the hover parameter to the quake chart and the rotate0 param to the alt.layer().

It somehow make sense, but I can imagine that people will expect that the parameters will work, no matter where added.

ChristopherDavisUCI commented 2 years ago

My vote is that c.add_parameter should only add to the given chart c, and not add to all sub-charts. I feel like this is what will be easiest to maintain. It won't break any existing code (since add_parameter doesn't exist yet), but it would mean that add_selection can't just get swapped out for add_parameter when updating existing code.

What do you think? I don't think there is a clear solution... this is just one possibility.

mattijn commented 2 years ago

From a VL perspective I agree.

But the current behavior of altair was quite convenient though, no need to think all this thorough.

I'm not sure if it is intended and by design that defined local and global parameters do not resolve or interplay smoothly with each other.

Maybe it's good to discuss this first a bit more at the VL repo with some minimal spec-examples, since the current cases are a bit scattered and lengthy (my bad).