WIP: update to Vega-Lite 5.2

[ChristopherDavisUCI]

The code here is pretty similar to #2517, but I wanted to redo some things and it seemed easiest to start with a new pull request. Also the conversation on the other one was getting pretty long.

Main changes from #2517:

• In the previous version, an expression like size_var + 3 created an Expression, but now it creates a Parameter. That is the only major change.
• As briefly discussed in the other PR, selection_point etc now create the entire Parameter.

Main changes overall (from the master branch):

• Include a vegalite/v5 folder with schema 5.2.0. The first commit is just a duplication of the vegalite/v4 folder. I hope that makes it easier to tell what changes were made.
• Move height/width/view from inner charts to the parent chart for LayerChart.
• Introduce parameters. Here are some examples (please ignore the course notes!).
• Moved most code out of Expression and into a new class called OperatorMixin. The goal is to use the same code for both something like expr + 3 (which should produce an Expression) and something like size_var + 3 (which should produce a Parameter). I'm really not sure if the way I accomplished this is natural or not.
• Added a fair amount of ad hoc code to keep the old selection syntax mostly working. I added many warnings.warn(message, DeprecationWarning), but they seem to all be hidden by default, so I have never successfully seen one of these messages displayed. The code would be a little cleaner if we didn't try to keep this old syntax.

If you see something that could be improved, please let me know, because I'm happy to learn about more efficient ways of doing things.

To do:

• When we are happy with the general syntax, I can write some documentation for parameters and add some examples. I also think it would be good to see if some old examples can be simplified. For example, this US population over time I think is more naturally made with a variable parameter as opposed to a selection parameter.
• Decide how forcefully we want to stop users from using the old selection syntax. My warnings.warn approach seems worthless at the moment since the messages don't get displayed by default.
• The only example that is raising an error is scatter plot with minimap. It is failing because it provides an explicit dictionary using the keyword selection, which in the new schema should be param. I think it's not worth the effort of writing code to try to make this example compile, and we should rewrite that example (only two words need to be changed).
• The other tests that I know of which are failing are some render examples (I haven't looked at those at all, nor have I updated Altair Saver) and test_spec_to_vegalite_mimebundle.

Thanks for any comments/requests!

[mattijn]

This looks really good. I like the OperatorMixin. Left a few inline comments. I think once Altair 4.2 is released we can start working on this PR to get accepted in master including passing of tests on this branch.

[ChristopherDavisUCI]

Thank you @mattijn! I haven't succeeded in finding the comments yet. I clicked on "Files Changed" above for this PR and scrolled through the altair/vegalite/v5/api.py and altair/expr/core.py files but didn't notice anything. Am I looking in the wrong place? Thanks again!

[mattijn]

My bad, didn't submit review.

[ChristopherDavisUCI]

Thanks @mattijn I made those and a few other small updates, including changing the scatter plot with minimap example. (That's the only example of a "breaking change" in version 5 that I know of.)

I'll maybe try writing some drafts of documentation for parameters, but I'm open to suggestions if there's something else that would be more helpful. (I don't think I can contribute anything useful on the JupyterLab front.)

[ChristopherDavisUCI]

Here is a nice small error I'm getting with the tests for this samplev52 branch.

import altair as alt
from altair_saver import save
from vega_datasets import data

source = data.cars()
brush = alt.selection(type='interval')

chart = alt.Chart(source).mark_point().encode(
    x='Horsepower:Q',
    y='Miles_per_Gallon:Q',
    color=alt.condition(brush, 'Cylinders:O', alt.value('grey')),
).add_selection(brush)

save(chart, 'chart.png')

If I instead just display the chart instead of trying to save it, it works fine. Or if I comment out the alt.condition line, it works fine.

The error that displays for me is

/Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:672
    return !!op.not;
                ^

TypeError: Cannot read property 'not' of undefined
    at isLogicalNot (/Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:672:17)
    at logicalExpr (/Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:953:9)
    at expression (/Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:14035:12)
    at /Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:9302:11
    at Array.map (<anonymous>)
    at wrapCondition (/Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:9299:39)
    at nonPosition (/Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:9621:12)
    at color (/Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:9679:10)
    at Object.symbols (/Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:15252:10)
    at parseLegendForChannel (/Users/christopherdavis/opt/anaconda3/envs/mattijn/share/vega-lite-cli/node_modules/vega-lite/build/vega-lite.js:15820:72)

and the traceback is

CalledProcessError                        Traceback (most recent call last)
/var/folders/8j/gshrlmtn7dg4qtztj4d4t_w40000gn/T/ipykernel_96872/2813738607.py in <module>
----> 1 save(chart, f'chart.png')

~/opt/anaconda3/envs/mattijn/lib/python3.9/site-packages/altair_saver/_core.py in save(chart, fp, fmt, mode, embed_options, method, suppress_data_warning, **kwargs)
    170     saver = Saver(spec, mode=mode, embed_options=embed_options, **kwargs)
    171 
--> 172     return saver.save(fp=fp, fmt=fmt)
    173 
    174 

~/opt/anaconda3/envs/mattijn/lib/python3.9/site-packages/altair_saver/savers/_saver.py in save(self, fp, fmt)
    119             raise ValueError(f"Got fmt={fmt}; expected one of {self.valid_formats}")
    120 
--> 121         content = self._serialize(fmt, "save")
    122         if fp is None:
    123             if isinstance(content, dict):

~/opt/anaconda3/envs/mattijn/lib/python3.9/site-packages/altair_saver/savers/_node.py in _serialize(self, fmt, content_type)
    112 
    113         if self._mode == "vega-lite":
--> 114             spec = self._vl2vg(spec)
    115 
    116         if fmt == "vega":

~/opt/anaconda3/envs/mattijn/lib/python3.9/site-packages/altair_saver/savers/_node.py in _vl2vg(self, spec)
     63         vl2vg = exec_path("vl2vg")
     64         vl_json = json.dumps(spec).encode()
---> 65         vg_json = check_output_with_stderr(
     66             [vl2vg], input=vl_json, stderr_filter=self._stderr_filter
     67         )

~/opt/anaconda3/envs/mattijn/lib/python3.9/site-packages/altair_saver/_utils.py in check_output_with_stderr(cmd, shell, input, stderr_filter)
    196     """
    197     try:
--> 198         ps = subprocess.run(
    199             cmd,
    200             shell=shell,

~/opt/anaconda3/envs/mattijn/lib/python3.9/subprocess.py in run(input, capture_output, timeout, check, *popenargs, **kwargs)
    526         retcode = process.poll()
    527         if check and retcode:
--> 528             raise CalledProcessError(retcode, process.args,
    529                                      output=stdout, stderr=stderr)
    530     return CompletedProcess(process.args, retcode, stdout, stderr)

CalledProcessError: Command '['/Users/christopherdavis/opt/anaconda3/envs/mattijn/bin/vl2vg']' returned non-zero exit status 1.

Is it obvious what kind of error this is? The possibilities I see:

• Something wrong with the way selections/conditions are currently defined in Altair.
• Something needs to be updated in Altair saver.
• Something wrong with how I have node packages installed. (I admit I don't know what I'm doing.)

[jakevdp]

It looks like you're saving via node-js, and I suspect node-js does not have the correct package versions installed.

[ChristopherDavisUCI]

Great, thank you! That has me down from about 35 tests failing to 8 tests failing.

I'm not sure how ill-advised this was, but I got the right versions installed by using npm install -g --force vega-lite vega-cli canvas. (It wasn't working without the -g or the --force.)

[ChristopherDavisUCI]

One observation about the current syntax in this PR is that you make a variable parameter using alt.parameter, but there is currently no "Pythonic" way to make a selection parameter using alt.parameter(select=...), so there is some asymmetry with the Vega-Lite syntax. Instead, currently the easy way to make a selection parameter is using alt.selection or alt.selection_interval or alt.selection_point. Is that an issue?

Some alternatives I can think of:

• Have a keyword like as_parameter that defaults to True in the alt.selection functions. One could then use as_parameter=False if they wanted to create the more low-level SelectionConfig, so it would look like x = alt.selection_point(as_parameter=False), and then one could construct a selection parameter using alt.parameter(select=x).
• Allow syntax like alt.parameter(select=alt.selection_interval()). This feels a little inelegant to me, because alt.selection_interval() is already a parameter, but implementing it would be easy. (We'd just check if the value passed for select was a selection parameter, and if it is, replace it with the corresponding selection config.) But maybe I'm wrong and that's not so bad?

But maybe nobody would ever use those alternatives?

What do you think? There's a balancing act between staying faithful to the Vega-Lite syntax on one hand, and having more concise code on the other hand (and not breaking the existing Altair syntax).

[mattijn]

I would follow Vega Lite API in here, see https://vega.github.io/vega-lite-api/api/#parameters and https://github.com/vega/vega-lite-api/blob/master/api/api.js#L124-L130. Basically meaning that the low-level SelectionConfig syntax cannot be created anymore using Altair. Its still allowed within Vega-Lite, to remain backward compatibility, but its on the list to be deprecated in the future.

[ChristopherDavisUCI]

Thanks @mattijn. The vega-lite api approach seems very similar to what we currently have (e.g., alt.parameter makes a variable parameter but doesn't naturally make a selection parameter), so I don't see any changes that need to be made.

Please let me know if you come across anything you think should be changed!

[ChristopherDavisUCI]

I've gotten myself a little confused about what's the right way to add parameters to a multiview chart.

Something like this works, where we add the parameter to the first chart and then refer to it in the second chart. An analogue also appears in the Vega-Lite documentation:

import altair as alt
from vega_datasets import data

cars = data.cars.url

brush = alt.selection_interval()

c1 = alt.Chart(cars).mark_circle().encode(
    x='Miles_per_Gallon:Q',
    y='Horsepower:Q',
).add_parameter(
    brush
)

c2 = alt.Chart(cars).mark_circle().encode(
    x='Miles_per_Gallon:Q',
    y='Horsepower:Q',
).transform_filter(
    brush
)

c1|c2

But something like this does not work. Again we add a parameter to the first chart and refer to it in the second:

year_slider = alt.binding_range(min=0, max=1000, step=5)
size_var = alt.parameter(bind=year_slider, value=200)

c1 = alt.Chart(cars).mark_circle(size=size_var).encode(
    x='Miles_per_Gallon:Q',
    y='Horsepower:Q',
).add_parameter(size_var)

c2 = alt.Chart(cars).mark_circle(opacity=size_var/1000).encode(
    x='Miles_per_Gallon:Q',
    y='Horsepower:Q',
)

c1|c2

If we instead add the parameter to the layer chart, then it works:

cars = data.cars.url

year_slider = alt.binding_range(min=0, max=1000, step=5)
size_var = alt.parameter(bind=year_slider, value=200)

c1 = alt.Chart(cars).mark_circle(size=size_var).encode(
    x='Miles_per_Gallon:Q',
    y='Horsepower:Q',
)

c2 = alt.Chart(cars).mark_circle(opacity=size_var/1000).encode(
    x='Miles_per_Gallon:Q',
    y='Horsepower:Q',
)

(c1|c2).add_parameter(size_var)

Do you see what the difference is between these two cases? Does this seem to be working as expected?

[mattijn]

It matters on which level the parameter are placed. I've experience in Vega-Lite that I had to add parameters in a nested concat chart, where it was not allowed to be defined on the top level. Will add an example soon. I'm not sure if this intended behaviour of VL5.

[mattijn]

The example I got is a bit more complex (Google Colab URL here):

import altair as alt
from vega_datasets import data

stocks = data.stocks.url
hover_1 = alt.selection_point(name="hover", nearest=True, on="mouseover", encodings=["x"])
hover_2 = alt.selection_point(name="hover", on="mouseover", encodings=["x"])
highlight = alt.selection_point(name="highlight", on="mouseover", clear="mouseout")
input_dropdown = alt.binding_select(options=[[0,300], [0,600]], name='fill domain')
param_domain = alt.param(value=[0, 600], bind=input_dropdown)

line = alt.Chart(height=150).mark_line(point=True).encode(
    x=alt.X("date:N", timeUnit="year"),
    y=alt.Y("mean(price):Q"),
    color="symbol:N"
)
rule = alt.Chart().mark_rule(tooltip=True).encode(
    x=alt.X("date:N", timeUnit="year"),
    color=alt.condition(hover_1, alt.value("black"), alt.value("transparent"), 
                        empty=False)

).add_params(hover_1)

rect = alt.Chart().mark_rect(strokeWidth=1.2, tooltip=True).encode(
    x=alt.X("date:T", timeUnit="year"),
    y=alt.Y("symbol:N"),
    fill=alt.Y("mean(price):Q", scale=alt.Scale(domain=param_domain)),
    stroke=alt.condition(highlight, alt.value("black"), alt.value(None), 
                        empty=False)
).add_params(hover_2, highlight) 

alt.concat(
    (line + rule), rect, data=stocks, columns=1
).resolve_scale(
    fill='independent', 
    color='independent', 
    x='shared'
).add_params(
  param_domain,
).configure_scale(bandPaddingInner=0.0, bandPaddingOuter=0.07)

If I move the highlight parameter from the rect to the concat it will give the following error:

Javascript Error: Duplicate signal name: "highlight_tuple"

Why is this not allowed?

But if I remove the param_domain parameter from the chart and only add highlight to the concat parameters as an add_selection it will work (see colab for full examples).

When looking to the Vega-Lite definitions I observe that the behaviour of add_selection is different than add_parameter. The add_selection on the concat add the highlight parameter to the correct rect parameters. How does it know it should do this?

Wrong VL-editor spec: Open the Chart in the Vega Editor | updated

Right VL-editor spec: Open the Chart in the Vega Editor

[jakevdp]

I'm not sure, but it seems suspicious that you have two parameters with the same name ("hover"). I believe parameter names should be unique

[mattijn]

Apologies, the VL-spec didn't reflect the text, I updated the spec (Open the Chart in the Vega Editor). There is just a single highlight parameter defined.

It seems that for concatenated views some of the parameters cannot be defined globally/top-level/low-level.

There are also two nested hover parameter defined for each concatenated view, by using the same name, they are connected to the same signal even though the behaviour for each view is slightly different (in top view, hover_1, there is nearest=True).

All in all, I think I observe the following behaviour:

• Parameters used as Expression strings and for data extents can be defined top-level, where usage is OK in nested views.
• Parameters used as Predicates need to be defined on each view, but by using the same parameter name one can define cross-view interaction.

[mattijn]

I'm wrong. Predicates can be defined top-level and still be used in layers within concatenated views, Open the Chart in the Vega Editor.
I will file an issue at the VL-repo, see vega/vega-lite#7854

[jakevdp]

Yeah, we should bump the version to 5.0.0 at some point before we release, but that doesn't have to happen now.

Also, we could probably delete all the v3 code.

I'm also thinking of deleting the altair.vega wrappers for the next major release, because I've never heard of anyone using them.

More examples and documentation would also be very welcome!

That could all be done after we merge this, though.

[ChristopherDavisUCI]

Thanks @jakevdp, that all sounds good. I'll watch for the merge!

[joelostblom]

Super excited to see this being on its way to a merge! @jakevdp What do you think about including the autocomplete change I suggested in #2528 (comment) to hide the deprecated methods as @mattijn originally brought up in #2528 (comment) (and the two following comments).

This is minor and I by no means want it to hold back a merge, but I thought I would mention it to double check since I agree that seeing the deprecated methods in the completion is somewhat confusing.

[jakevdp]

My only hesitation with #2528 (comment) is that the module is referencing itself in a partially-imported state, and I don't know if there are potentially problematic edge-cases there.

[ChristopherDavisUCI]

Thanks @joelostblom for the reminder and @jakevdp for the comment. My vote would be to not include this autocomplete change for now and maybe consider it again if things seem to be working.

[joelostblom]

Thanks both! Erring on the side of caution sounds appropriate to me as well, so let's consider the modified tab completion for later if needed. I saw that you added the deprecation warnings @ChristopherDavisUCI so that still clearly communicates the deprecated state of these functions.

[mattijn]

Hey @ChristopherDavisUCI, I'm stuck here. I've the feeling when an expr is defined within a parameter its not properly serialized into correct VL using this branch.
Observe the following in Altair:

import altair as alt

data_wind = [
    {'winddirection': 0, 'label': 'North'},
    {'winddirection': 90, 'label': 'East'},
    {'winddirection': 180, 'label': 'South'},
    {'winddirection': 270, 'label': 'West'},
]

# off-topic: I wish I could define names for data sources easier
source_wind = alt.DataSource(alt.InlineData(values=data_wind, name='source_wind'))

theta_shift = alt.parameter(expr="-PI/length(data('source_wind'))")

chart = alt.Chart(source_wind).mark_arc(
    tooltip=True,
    thetaOffset=theta_shift
).encode(
    theta='winddirection:N',
    color='label:N'
).add_parameter(theta_shift)
chart

Checking the dict:

chart_dict = chart.to_dict()
chart_dict

Changing epxr to param name, is success:

chart_dict['mark']['thetaOffset']['expr'] = 'parameter027'
alt.Chart.from_dict(chart_dict)

Open the Chart in the Vega Editor

[ChristopherDavisUCI]

Thanks @mattijn! I'm swamped with meetings today but will check this out and get back to you Friday.

[mattijn]

Perfect! Funny enough, if I do: thetaOffset=theta_shift+0, it does works. Its still not referenced by param name, but at least a correct expr. Will the serialization of thetaOffset=theta_shift+theta_shift also go well?

[ChristopherDavisUCI]

@mattijn, can I ask, how do you decide what the "correct" serialization should be? Do you plug it into the Vega-Lite editor and see if it gives any warnings or errors? Do you look at the Vega-Lite docs? Do you look at the typescript source code? Do you look at the json schema? Something else? For most cases I use the Vega-Lite docs, but for edge cases (like how should a parameter be specified in a multi-view chart) it's very unclear to me where I should be looking.

[mattijn]

I use the Vega editor as starting point.
In this case, I would assume that the following Altair:

1. thetaOffset=theta_shift is resolved in VL as "thetaOffset": {"expr": "parameter027"}
2. thetaOffset=theta_shift + 0 is resolved in VL as "thetaOffset": {"expr": ("parameter027 + 0)"}
3. thetaOffset=theta_shift + theta_shift is resolved in VL as "thetaOffset": {"expr": ("parameter027 + parameter027)"}

But currently the following happens:

1. thetaOffset=theta_shift is resolved in VL as "expr": "\"-PI/length(data('source_wind'))\""
2. thetaOffset=theta_shift + 0 is resolved in VL as "expr": "(-PI/length(data('source_wind')) + 0)"
3. thetaOffset=theta_shift + theta_shift is resolved in VL as "(-PI/length(data('source_wind')) + -PI/length(data('source_wind')))"

If I define the parameter as value and not as an expr (eg. theta_shift = alt.parameter(value=-0.78)) it is currently serialized with this logic already, namely the result is "expr": "(parameter027 + parameter027)"

[mattijn]

I've the feeling the behavior seen is because of this line: https://github.com/altair-viz/altair/pull/2528/files#diff-bbe4e187b18d242a366c820d023afd89041759cb96e4ec66c3f34559a72c2f9dR177.

[mattijn]

I just found out it is not necessary to use an .add_parameter(theta_shift). I can just define an alt.parameter and use it in the spec. Is this intended? (open question).

import altair as alt
from altair.expr import PI, length, data

data_wind = [
    {'winddirection': 0, 'label': 'North'},
    {'winddirection': 90, 'label': 'East'},
    {'winddirection': 180, 'label': 'South'},
    {'winddirection': 270, 'label': 'West'},
]

source_wind = alt.DataSource(alt.InlineData(values=data_wind, name='source_wind'))

theta_shift = alt.parameter(expr=(-PI/length(data('source_wind'))))

chart = alt.Chart(source_wind).mark_arc(
    tooltip=True,
    thetaOffset=theta_shift
).encode(
    theta='winddirection:N',
    color='label:N'
)
chart#.to_dict()

[jakevdp]

Sorry for this falling off my radar. It looks like there are still some questions about corners of the new APIs... still, does anyone object to me clicking merge so we can more easily test and iterate on the new version?

[mattijn]

No objections, let's iterate further from main branch (or rc1 🤩)

[ChristopherDavisUCI]

Hi @jakevdp, please go ahead! I should have time to look over these issues @mattijn mentioned over the next few days.

[ChristopherDavisUCI]

Two quick questions:

Is there any downside to deleting the Altair repository that I forked? I find it difficult to keep these things organized.

When I respond to @mattijn's questions, should I do that in a new GitHub issue or PR, or here?

[mattijn]

@ChristopherDavisUCI, you can respond at #2573, and if we find an answer, can create a PR afterwards.
Also created #2574 for the auto-completion of deprecated methods.

[mattijn]

Regarding:

Is there any downside to deleting the Altair repository that I forked? I find it difficult to keep these things organized.

Up to you. Clean is nice. You'll have to create a new fork for a new PR.

[joelostblom]

Wohoooo!!! It's so exciting to see this merged!! Thanks everyone for working on it, especially @ChristopherDavisUCI for championing this large undertaking 🚀 🌟 🏆