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

Update from Vega-Lite 5.2.0 to 5.6.1 #2871

Merged
merged 11 commits into from
Feb 24, 2023
Merged

Update from Vega-Lite 5.2.0 to 5.6.1 #2871

merged 11 commits into from
Feb 24, 2023

Conversation

ChristopherDavisUCI
Copy link
Contributor

Based on the release notes for Vega-Lite 5.6.0, I tried setting zero=False as the scale configuration default.

import altair as alt
from vega_datasets import data

cars = data.cars.url

c = alt.Chart(cars).mark_circle().encode(
    x = "Weight_in_lbs:Q",
    y = "Acceleration:Q"
).properties(
    height=100,
    width=100
).configure_scale(
    zero=False,
)

No errors are raised (even when using validate="deep"), whereas errors are raised if I use this code with the current version on GitHub. I take that as a good sign.

On the other hand, the displayed chart doesn't reflect this change:
chart

Maybe vega-embed is not matching vega-lite 5.6 yet? That's my first guess.

@ChristopherDavisUCI
Copy link
Contributor Author

I haven't investigated the failing tests, but my first guess is that 5.2 needs to be updated to 5.6 in some other places, like altair_viewer? Maybe this comment is relevant?

If it's failing because of a reason I can help with, please let me know!

@mattijn
Copy link
Contributor

mattijn commented Feb 4, 2023

We've to supersede this PR: altair-viz/altair_viewer#51 in altair_viewer.

@joelostblom
Copy link
Contributor

Thanks @ChristopherDavisUCI ! The changes here look good to me, but let's wait until we have updated altair_viewer to support 5.6 as @mattijn mentioned, so that we can be sure that the tests pass.

@mattijn mattijn changed the title Update from Vega-Lite 5.2.0 to 5.6.0 Update from Vega-Lite 5.2.0 to 5.6.1 Feb 23, 2023
@mattijn
Copy link
Contributor

mattijn commented Feb 23, 2023

@ChristopherDavisUCI, I bumped the main repository of altair_viewer to vl v5.6.1, can you check why the 4 failing tests in this PR are failing?

@ChristopherDavisUCI
Copy link
Contributor Author

Hi @mattijn, it looks like at least one of the issues is the same one as reported here: #2856 (same example, same error). Do you see why that error is coming up on this PR and not on every PR?

Another issue involves mimebundle, but at first glance I don't have anything to add.

@mattijn
Copy link
Contributor

mattijn commented Feb 24, 2023

As raised in vega/vega-lite#8685:

This specification renders in Altair.
But the interaction only works once.

import altair as alt
from vega_datasets import data

source = data.cars.url

bind_checkbox = alt.binding_checkbox(name='checkbox')
param_checkbox = alt.selection_point(bind=bind_checkbox)

cond_checkbox = alt.condition(
    param_checkbox,
    alt.value(25),
    alt.Size('Acceleration:Q')
)

chart = alt.Chart(source).mark_circle().encode(
    x='Horsepower:Q',
    y='Miles_per_Gallon:Q',
    size=cond_checkbox
).add_params(param_checkbox)
chart

image

Once trying to save the chart as png or svg, it will provide the same error as when you open the Chart in the Vega Editor.

chart.save('chart.svg')
ValueError: Vega to SVG conversion failed:
Error: Unrecognized signal name: "param_18_tuple_fields"
    at b (https://cdn.skypack.dev/-/vega-util@v1.17.0-uRskU0IBL2vWCP4Va8OC/dist=es2020,mode=imports,min/optimized/vega-util.js:1:324)
    at ue.getSignal (https://cdn.skypack.dev/-/vega-parser@v6.1.4-PbzshukasZitnbnp29GQ/dist=es2020,mode=imports,min/optimized/vega-parser.js:1:43806)
    at https://cdn.skypack.dev/-/vega-functions@v5.12.1-G6oHWIVrVbJOnq67V4GW/dist=es2020,mode=imports,min/optimized/vega-functions.js:1:11264
    at Array.forEach (<anonymous>)
    at Nn (https://cdn.skypack.dev/-/vega-functions@v5.12.1-G6oHWIVrVbJOnq67V4GW/dist=es2020,mode=imports,min/optimized/vega-functions.js:1:11229)
    at Pe (https://cdn.skypack.dev/-/vega-parser@v6.1.4-PbzshukasZitnbnp29GQ/dist=es2020,mode=imports,min/optimized/vega-parser.js:1:8014)
    at https://cdn.skypack.dev/-/vega-parser@v6.1.4-PbzshukasZitnbnp29GQ/dist=es2020,mode=imports,min/optimized/vega-parser.js:1:39038
    at Array.forEach (<anonymous>)
    at hn (https://cdn.skypack.dev/-/vega-parser@v6.1.4-PbzshukasZitnbnp29GQ/dist=es2020,mode=imports,min/optimized/vega-parser.js:1:39027)
    at al (https://cdn.skypack.dev/-/vega-parser@v6.1.4-PbzshukasZitnbnp29GQ/dist=es2020,mode=imports,min/optimized/vega-parser.js:1:39762)

@mattijn
Copy link
Contributor

mattijn commented Feb 24, 2023

I temporarily removed the sub chart that is binded to alt.binding_checkbox() in https://altair-viz.github.io/gallery/multiple_interactions.html (bottom-right). Now checks are passing. Once it is fixed in VL (vega/vega-lite#8685) then we can turn on this sub chart again.

@mattijn mattijn requested a review from joelostblom February 24, 2023 09:32
@mattijn
Copy link
Contributor

mattijn commented Feb 24, 2023

You agree @joelostblom?

@joelostblom
Copy link
Contributor

I think that instead of removing the chart, we could change it based on the comment in vega/vega-lite#4870 (comment):

- param_checkbox = alt.selection_point(bind=bind_checkbox)
+ param_checkbox = alt.param(bind=bind_checkbox)

This makes the example work for me, maybe we should update all checkpoint examples to use this approach?

@mattijn
Copy link
Contributor

mattijn commented Feb 24, 2023

Good memory! Make sense👍

@ChristopherDavisUCI
Copy link
Contributor Author

Hi @joelostblom @mattijn I'm going to be traveling the next few days, so please go ahead and make any changes if you see anything that should be done soon. Otherwise I should be back on schedule Monday.

@mattijn
Copy link
Contributor

mattijn commented Feb 24, 2023

Updated as you suggested @joelostblom. Tests pass now, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants