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

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

Closed
mattijn opened this issue Apr 26, 2022 · 5 comments
Closed
Labels

Comments

@mattijn
Copy link
Contributor

mattijn commented Apr 26, 2022

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 vega/vega-lite#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?

@mattijn mattijn added the bug label Apr 26, 2022
@ChristopherDavisUCI
Copy link
Contributor

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
Copy link
Contributor

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:
vega/vega-lite#7812
vega/vega-lite#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
Copy link
Contributor Author

mattijn commented May 1, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

mattijn commented May 8, 2022

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

@mattijn mattijn closed this as completed Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants