-
Notifications
You must be signed in to change notification settings - Fork 795
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
WIP: Lifting parameters to the top level #2684
Conversation
The main work has not been done yet: writing the function `_combine_subchart_params`.
@mattijn I haven't worked on the Vega-Lite issues you raised in #2671 (comment). If this code seems to be working for you (and not too complicated), I will start thinking about those issues. Also if we decide this is working as expected, I will go through the 8 (?) failing tests and either change the tests or update my code. |
Great @ChristopherDavisUCI! Looks more readable. I like the changes. You might align these definitions: with the changes you introduced here: https://github.com/altair-viz/altair/blob/70682835bd4d80e31a378890f6f2274501cf2849/altair/vega/v5/schema/__init__.py including a run of the !python -m pip install git+https://github.com/ChristopherDavisUCI/altair.git@liftparams
import altair as alt
alt.SCHEMA_VERSION gives me I checked a few of these failing tests and they seem to be related to the more recent discussion we had regarding the behaviour of layered params. These tests seems compact and interesting and I did not really see a case that require changing the tests assertion of these failing tests (excluding |
Thanks @mattijn! I see now that I had updated the Vega version in the wrong place; it should be corrected now. I also updated vega-lite to 5.5.0. Does that sound like what you had in mind? (It looks like the Vega-Lite editor uses vega-lite 5.2.0.) I'll take a closer look at the failing tests soon. |
@mattijn Here's one concern I have related to the failing tests. If I understand right, the failing layer chart test, for example, asserts that adding a selection parameter to a LayerChart should be the same as adding a selection parameter to the first chart in the If you think that sort of hypothetical future issue could be fixed easily enough later, I'll go ahead and try to add functionality so there is more flexibility in placing selection parameters on top level charts. |
I think I understand what you mean. I think I've raised and discuss this in this VL-issue, vega/vega-lite#7854 (comment). At the moment we can assume that multiple selection parameter placements within a layered chart results in potentially interference and that therefor a single layered object can only have 1 I see it as a single name definition on the full layer-object, like the following in VL-json: "layer": [
"name":"view_0",
{},
{}
] But this type of structure is not allowed within VL. We might still opt for defining this as such and then change it (including a comment) subsequently into: "layer": [
{"name":"view_0"},
{}
] Then when this behavior is potentially changing in the future, it is potentially easier to adapt to these changes. This approach might also help in presenting a user-friendly error when defining two different selection parameters in the layer or for resolving two equal defined selection parameters in the layer (see also #2671 (comment)). |
Edit As I think about it, maybe the parameter should only be distributed if it doesn't have a Hi @mattijn, I fixed three of the failing tests (for concat, hconcat, vconcat) by doing the following:
Does that all sound reasonable? An alternative option to resetting the |
I don't see a problem for resetting the
Hm, not sure.. base = alt.Chart("data.csv").mark_point()
selection = alt.selection_point() Then for concatenating, this is proper VL-style: chart1 = alt.concat(base.add_params(selection), base.add_params(selection)) This is not proper VL-style, but allowed by Altair:
So, doing a redistribution there for the Thinking a bit more, I do agree that one cannot just blindly distribute all params to all objects within alt.concat(alt.layer(base, base), alt.layer(base, base)).add_params(selection) But then it should be somehow recognize to do: alt.concat(alt.layer(base.add_params(selection), base), alt.layer(base.add_params(selection), base)) Edit: the tests I considered: !python -m pip install git+https://github.com/ChristopherDavisUCI/altair.git@liftparams
import altair as alt
from vega_datasets import data
def test_compound_add_selections(charttype):
base = alt.Chart("data.csv").mark_point()
selection = alt.selection_point()
alt.Chart._counter = 0
chart1 = charttype(base.add_params(selection), base.add_params(selection))
alt.Chart._counter = 0
chart2 = charttype(base, base).add_params(selection)
print(f'{chart1.to_dict()}\n{chart2.to_dict()}')
#assert chart1.to_dict() == chart2.to_dict()
def test_repeat_add_selections():
base = alt.Chart("data.csv").mark_point()
selection = alt.selection_point()
chart1 = base.add_params(selection).repeat(list("ABC"))
chart2 = base.repeat(list("ABC")).add_params(selection)
print(f'{chart1.to_dict()}\n{chart2.to_dict()}')
#assert chart1.to_dict() == chart2.to_dict()
def test_facet_add_selections():
base = alt.Chart("data.csv").mark_point()
selection = alt.selection_point()
chart1 = base.add_params(selection).facet("val:Q")
chart2 = base.facet("val:Q").add_params(selection)
print(f'{chart1.to_dict()}\n{chart2.to_dict()}')
#assert chart1.to_dict() == chart2.to_dict()
def test_layer_add_selection():
base = alt.Chart("data.csv").mark_point()
selection = alt.selection_point()
chart1 = alt.layer(base.add_params(selection), base)
chart2 = alt.layer(base, base).add_params(selection)
print(f'{chart1.to_dict()}\n{chart2.to_dict()}')
#assert chart1.to_dict() == chart2.to_dict()
def test_interactive_layered_crossfilter():
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)),
#x=alt.X('distance', 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"])
#assert chart
print(f'{chart.to_dict()}')
for charttype in [alt.concat, alt.hconcat, alt.vconcat]:
print(charttype)
test_compound_add_selections(charttype)
test_repeat_add_selections()
test_facet_add_selections()
test_layer_add_selection()
test_interactive_layered_crossfilter() |
Reverting back to the old definitions of `add_params`. Also resetting `_counter` in a test.
Hi @mattijn, I think the only failing tests at this point are the two related to Do you see a "parameters at the top level with named views" version of this Vega-Lite schema that is allowed? https://vega.github.io/vega-lite/examples/interactive_layered_crossfilter.html Or should we just resign ourselves to our strategy not working for this example at the moment? |
Great progress @ChristopherDavisUCI! Well done. I tried a few things and I think this is working pretty good. Still a question: Do you think this is something that should be resolved within this PR? |
I had another go using |
Those six examples you provided vega/vega-lite#6890 (comment) are super helpful @mattijn. I haven't wrapped my head around the different scenarios yet. Just as a big-picture overview, do you think your option 6 will give us a way to produce this example with the (At least in this PR, I think we should try to consistently have |
Yes I think so. We should at least be able to get a Vega-Lite spec that compiles. The interaction and axis will not be perfect, since these VL-bug(s): vega/vega-lite#8446, which is hopefully a duplicate of vega/vega-lite#8348, but we should be able to get a chart that renders. If we instead focus on the Altair version of option 6, we can test better. !python -m pip install git+https://github.com/ChristopherDavisUCI/altair.git@liftparams
import pandas as pd
import altair as alt
source = pd.read_csv('https://unpkg.com/[email protected]/data/weather.csv')
highlight = alt.selection_point(
nearest=True, on="mouseover", encodings=["x"], clear="mouseout"
)
rule = (
alt.Chart()
.mark_rule()
.encode(
x=alt.X("month(date):T"),
color=alt.value("black"),
opacity=alt.condition(highlight, alt.value(1), alt.value(0), empty=False),
)
)
line = (
alt.Chart()
.mark_line(point=True)
.encode(
x=alt.X("month(date):T"),
y=alt.Y("mean(precipitation):Q"),
#y=alt.Y(alt.repeat('repeat'), aggregate='mean'),
color=alt.Color("location:N"),
)
)
alt.layer(rule, line, data=source, height=200, width=200).add_params(
highlight
)#.repeat(repeat=["temp_max", "precipitation", "wind"]) If I add the SchemaValidationError: Invalid specification
altair.vegalite.v5.schema.channels.OpacityValue, validating 'additionalProperties'
Additional properties are not allowed ('param', 'empty' were unexpected)
alt.RepeatChart(...) This should compile, so we still miss something in the |
Hi @mattijn, thanks for the continued testing! I think I got the RepeatChart issue fixed. I haven't thought yet about your scenarios 3 and 4 that you mentioned in #2671 (comment) (but can confirm they're still not working). I will think about that now. Could you please take another look and see if things are now behaving mostly as you expected? Is it obvious to you which tests have failed in the Python 3.9 build? On my end they seem to be passing. |
Tests on Github Actions are failing because this PR in the |
Thanks as always for the feedback @mattijn. I think your four scenarios from #2671 (comment) are currently working. |
Nice as always @ChristopherDavisUCI! import altair as alt
from vega_datasets import data
iris = data.iris.url
hover = alt.selection_point(on='mouseover', nearest=True)
base = alt.Chart().encode(
x='petalLength:Q',
y='petalWidth:Q',
color='species:N' # alt.condition(hover, 'species:N', alt.value('lightgray'))
).properties(
width=180,
height=180,
).add_params(hover)
points = base.mark_point()
text = base.mark_text(dy=-5).encode(
text = 'species:N',
opacity = alt.value(1) # alt.condition(hover, alt.value(1), alt.value(0), empty=False)
)
chart = alt.layer(points, text, data=iris).facet(
'species:N',
) # .add_params(hover)
chart # .to_dict() Returns:
Not sure where I should add my data, I tried within And the second layered facet chart including defined parameters: import altair as alt
from vega_datasets import data
iris = data.iris.url
hover = alt.selection_point(on='mouseover', nearest=True)
base = alt.Chart().encode(
x='petalLength:Q',
y='petalWidth:Q',
color=alt.condition(hover, 'species:N', alt.value('lightgray')) # 'species:N'
).properties(
width=180,
height=180,
).add_params(hover)
points = base.mark_point()
text = base.mark_text(dy=-5).encode(
text='species:N',
opacity=alt.condition(hover, alt.value(1), alt.value(0), empty=False) # alt.value(1)
)
chart = alt.layer(points, text).facet(
'species:N',
data=iris
).add_params(hover)
chart # .to_dict()` Gives me:
|
Thanks @mattijn! These "data missing" error messages usually mean the parameters are in the wrong place, and that seems to be the case here. I'll take a look. |
Hi @mattijn, luckily it seems like the same fix that worked for LayerChart also works in this case. Do your examples now seem to be working? Please keep sending any non-working examples you can come up with! |
Hmm, do you see what's wrong with the json that results from your example? At first glance I would have expected this json to be correct. Open the Chart in the Vega Editor |
Nope, reported here: vega/vega-lite#8452. Including a grouped issue regarding all the issues we have for the |
I discovered two more issues that are not working: First, !python -m pip install git+https://github.com/ChristopherDavisUCI/altair.git@liftparams
import altair as alt
source = 'https://unpkg.com/[email protected]/data/weather.csv'
# interval = alt.selection_interval(encodings=['x'], bind='scales')
line = (
alt.Chart(height=100)
.mark_bar(point=True)
.encode(
x=alt.X("date:T"),
y=alt.Y("precipitation:Q"),
color=alt.Color("location:N"),
)
)
alt.vconcat(line, line, data=source).interactive(bind_y=False) # .add_params(interval)
Second, I get a duplicate signal name error with the following: highlight = alt.selection_point(
nearest=True, on="mouseover", encodings=["x"], clear="mouseout"
)
interval = alt.selection_interval(encodings=['x'], bind='scales')
line = (
alt.Chart(height=100)
.mark_line(point=True)
.encode(
x=alt.X("month(date):T"),
y=alt.Y("mean(precipitation):Q"),
color=alt.Color("location:N"),
)
) # .interactive(bind_y=False) # add_params(interval)
rule = (
alt.Chart()
.mark_rule()
.encode(
x=alt.X("month(date):T"),
color=alt.value("black"),
opacity=alt.condition(highlight, alt.value(1), alt.value(0), empty=False),
)
)
tick = (
alt.Chart()
.mark_tick(point=True)
.encode(
x=alt.X("month(date):T"),
y=alt.Y("mean(precipitation):Q"),
color=alt.Color("location:N"),
)
)
alt.vconcat(line, tick + rule, data=source).add_params(
highlight
)
|
Thanks @mattijn I will check those out! I always appreciate these bug examples. By the way, it's slightly easier for me when you use the "url" data syntax, because then the json that's created is much more manageable. So if it's ever easy to make that switch, please do! |
Will do! |
Thanks for changing the data! I will try to understand the syntax so I can do that myself in the future. Are we sure vconcat and hconcat should have an |
For me it make sense and the VL-scheme is OK with it. If defined on the concat level, the axis interaction will be done on both charts: Open the Chart in the Vega Editor This is different compare to when defining on each sub-chart (which works already): Open the Chart in the Vega Editor Even with unequal axis name and type, this is OK on the concat: Open the Chart in the Vega Editor BTW, it's just a shortcut to the parameter variant (which works already):
For the second example regarding the duplicate signal, I think we had this issue before. |
Okay cool, should be no problem to add that I haven't taken a look at the second example yet but will soon. |
The interaction is bound between the charts (as opposed to another possibility, where the interaction would be added to each constituent chart separately).
Hi @mattijn, could you please check and see if the concat Regarding your second example, to me (maybe this is the same as you wrote) it seems like this is an issue on the Vega-Lite side, not the Altair side? Or at least I thought this kind of parameter use was allowed. (As a mnemonic, I believe your example has the form |
Yes, that works great! For the second chart, if I do:
it does work. But if I also add a highlight condition to the line then I run into issues again. There is some peculiarity with source = "https://unpkg.com/[email protected]/data/weather.csv"
highlight = alt.selection_point(
name="hover",
on="mouseover",
nearest=True,
encodings=["x"],
empty=False,
clear="mouseout",
)
top_base = alt.Chart(height=100, width=200).encode(
x=alt.X("month(date):T"),
y=alt.Y("mean(precipitation):Q"),
color=alt.condition(highlight, alt.Color("location:N"), alt.value("gray")),
)
line = top_base.mark_line()
circle = top_base.mark_circle(size=100)
rule = (
alt.Chart(height=100, width=200)
.mark_rule(size=6)
.encode(
x=alt.X("month(date):T"),
color=alt.value("black"),
opacity=alt.condition(highlight, alt.value(1), alt.value(0)),
)
)
tick = (
alt.Chart()
.mark_tick(thickness=5)
.encode(
x=alt.X("month(date):T"),
y=alt.Y("mean(precipitation):Q"),
color=alt.Color("location:N"),
)
)
alt.vconcat((line + circle), (tick + rule), data=source).add_params(highlight) Let's leave it at that for now. One more question regarding docstrings, probably better for another PR, but I will still mention it. Currently the docstrings for some new introduced parameter methods are rather limited: Where in the above example I've defined it as such: highlight = alt.selection_point(
name="hover",
on="mouseover",
nearest=True,
encodings=["x"],
empty=False,
clear="mouseout",
) How can one know about this without docstrings? |
Let me create an issue on the docstrings and the current limitation on repeat within the altair repo and then we can wait here until altair-viz/altair_viewer#51 is merged so the Github Actions in this PR can be properly do their jobs (cc: @jakevdp). |
Hi @mattijn, do you know, what is the analogue of |
It is I modified the last chart under this section from the VL-docs here https://vega.github.io/vega-lite/docs/bind.html#scale-binding as an example, see: Open the Chart in the Vega Editor. |
Thanks as always @mattijn! I might start with a fresh PR once things seem to be working, but I think the latest commit has the repeat names working for plain repeats as well as repeats specifying "row" and/or "column". (Should we be supporting anything else, like @mattijn Can you work your usual magic and find charts that are not behaving as expected? Is the following supposed to work? It's not at the moment. It produces this Chart in the Vega Editor
|
Your last spec won't work as was concluded in #2684 (comment). This spec: import altair as alt
from vega_datasets import data
source = data.movies.url
alt.Chart(source).mark_line().encode(
x=alt.X(alt.repeat("column"), bin=True),
# x=alt.X("IMDB_Rating:Q", bin=True),
y=alt.Y(
alt.repeat("layer"), aggregate="mean", title="Mean of US and Worldwide Gross"
),
color=alt.ColorDatum(alt.repeat("layer")),
).repeat(
layer=["US_Gross", "Worldwide_Gross"],
column=["IMDB_Rating", "Rotten_Tomatoes_Rating"],
) # .interactive() Gives: ValueError: layer argument cannot be combined with row/column argument. While it is allowed in VL: Open the Chart in the Vega Editor. Technically not related to this PR though. I cannot test what the behaviour of This spec also gives the same error on duplicate signals: #import altair as alt
source = 'https://unpkg.com/[email protected]/data/weather.csv'
highlight = alt.selection_point(
nearest=True, on="mouseover", encodings=["x"], clear="mouseout"
)
rule = (
alt.Chart()
.mark_rule()
.encode(
x=alt.X(alt.repeat('row'), bin=True),
color=alt.value("black"),
opacity=alt.condition(highlight, alt.value(1), alt.value(0), empty=False),
)
)
line = (
alt.Chart()
.mark_line(point=True)
.encode(
x=alt.X(alt.repeat('row'), bin=True),
y=alt.Y(alt.repeat('column'), bin=True),
color=alt.Color("location:N"),
)
)
alt.layer(line, rule, data=source, height=200, width=200).add_params(
highlight
).repeat(column=["precipitation", "wind"], row=["temp_max", "precipitation"])#.to_dict() But changing the order or The tests I covered so far: https://colab.research.google.com/drive/16oqde86OsNFBtVpa4x0p7yMEyWKcUcAP?usp=sharing I think we are done here. |
Awesome, thanks again @mattijn. I'm hoping to create a new PR so I can clean up some of the logic/ordering of the commits. |
Closing this in favor of #2702. |
Updated version of #2671. I believe the functionality is the same, but the code is a little cleaner. @mattijn As always I am happy for any comments!
Here was the description of #2671:
This is a draft version (some tests are currently failing) of the strategy suggested by @arvind and @mattijn here for dealing with parameters in multi-view Altair charts. We lift all parameters to the top level. In the case of selection parameters, we give the parameter a "views" property to indicate where the selections should be happening.