Closed dangotbanned closed 1 month ago
@mattijn following your feedback in https://github.com/vega/altair/pull/3536#issuecomment-2365430472 I'm trying to write a helpful description but having a hard time, without either:
This is otherwise ready for review, so I wanted to ask ahead to see what content you'd like to see there
Thanks for requesting a review. You can help me explaining how I can review this. Do I have to run write_expr_module(source_url="static", output=EXPR_FILE)
myself or inspect expressions by importing altair.expr.xx
? Thanks for the work here.
Thanks for requesting a review. You can help me explaining how I can review this. Do I have to run
write_expr_module(source_url="static", output=EXPR_FILE)
myself or inspect expressions by importingaltair.expr.xx
? Thanks for the work here.
@mattijn
So everything should work exactly the same as the current alt.expr
.
If you check out this branch - the changes should be reflected in two ways:
alt.expr.xxx()
TypeError
(s) if you try to call a method without enough arguments
The actual code generation you could call yourself, but this is already in the CI. I suppose you could check the output is the same?
I get proper signatures when doing
import altair as alt
alt.expr.scale()
Expected 2 more positional arguments (method) def scale( name: IntoExpression, value: IntoExpression, group: IntoExpression = None, / ) -> Expression Applies the named scale transform (or projection) to the specified value. The optional group argument takes a scenegraph group mark item to indicate the specific scope in which to look up the scale or projection.
But not when doing:
import altair.expr as ae
ae.scale()
"scale" is not a known attribute of module "altair.expr" (function) scale: Unknown
See animated gif:
Is that something that is expected? Or was it naive to expect that something as such could work?
Is that something that is expected? Or was it naive to expect that something as such could work?
@mattijn I managed to get all of these working correctly:
# ruff: noqa: B018
import altair as alt
# import altair.expr as ae
from altair import expr
from altair import expr as ae
from altair.expr import expr as expr_cls
alt.expr.scale
expr.scale
ae.scale
expr_cls.scale
AFAIK this behavior isn't something I'd expect to have changed vs main
- but would be interesting if so
Thanks! It works as expected, but that also means it surface the following issue: This works
import polars as pl
import altair as alt
from altair import expr as ae
df = pl.DataFrame(
{
"x": [0.32, 0.86, 0.10, 0.30, 0.47, 0.0, 1.0, 0.91, 0.88, 0.12],
"color": list("ACABBCABBA"),
}
)
param_array = alt.param(name="my_array", value=["B", "A", "C"])
alt.Chart(df).mark_tick(thickness=10).encode(
x=alt.X("x"),
color=alt.Color("color").scale(domain=param_array)
).add_params(param_array)
But this will not:
param_array = alt.param(name="my_array", value=["B", "A", "C"])
param_sort = alt.param(name="my_sorted_array", expr=ae.sort(param_array))
alt.Chart(df).mark_tick(thickness=10).encode(
x=alt.X("x"),
color=alt.Color("color").scale(domain=param_sort)
).add_params(param_array, param_sort)
Javascript Error: Unrecognized function: sort This usually means there's a typo in your chart specification. See the javascript console for the full traceback.
And while ae.sort(param_array)
is now recognised as valid, since it is now auto-generated from the docs (great!), it is not yet released as part of the current accepted Vega version within Altair therefore raising an JavaScript error.
The compiled vega-lite specification looks correctly though:
{
"config": {"view": {"continuousWidth": 300, "continuousHeight": 300}},
"data": {"name": "data-1d2849971aacb1c06bb7718af005021c"},
"mark": {"type": "tick", "thickness": 10},
"encoding": {
"color": {
"field": "color",
"scale": {"domain": {"expr": "my_sorted_array"}},
"type": "nominal"
},
"x": {"field": "x", "type": "quantitative"}
},
"params": [
{"name": "my_array", "value": ["B", "A", "C"]},
{"name": "my_sorted_array", "expr": "sort(my_array)"}
],
"$schema": "https://vega.github.io/schema/vega-lite/v5.20.1.json",
"datasets": {
"data-1d2849971aacb1c06bb7718af005021c": [
{"x": 0.32, "color": "A"},
{"x": 0.86, "color": "C"},
{"x": 0.1, "color": "A"},
{"x": 0.3, "color": "B"},
{"x": 0.47, "color": "B"},
{"x": 0, "color": "C"},
{"x": 1, "color": "A"},
{"x": 0.91, "color": "B"},
{"x": 0.88, "color": "B"},
{"x": 0.12, "color": "A"}
]
}
}
But this will not:
...
And while
ae.sort(param_array)
is now recognised as valid, since it is now auto-generated from the docs (great!), it is not yet released as part of the current accepted Vega version within Altair therefore raising an JavaScript error.The compiled vega-lite specification looks correctly though:
{ "config": {"view": {"continuousWidth": 300, "continuousHeight": 300}}, "data": {"name": "data-1d2849971aacb1c06bb7718af005021c"}, "mark": {"type": "tick", "thickness": 10}, "encoding": { "color": { "field": "color", "scale": {"domain": {"expr": "my_sorted_array"}}, "type": "nominal" }, "x": {"field": "x", "type": "quantitative"} }, "params": [ {"name": "my_array", "value": ["B", "A", "C"]}, {"name": "my_sorted_array", "expr": "sort(my_array)"} ], "$schema": "https://vega.github.io/schema/vega-lite/v5.20.1.json", "datasets": { "data-1d2849971aacb1c06bb7718af005021c": [ {"x": 0.32, "color": "A"}, {"x": 0.86, "color": "C"}, {"x": 0.1, "color": "A"}, {"x": 0.3, "color": "B"}, {"x": 0.47, "color": "B"}, {"x": 0, "color": "C"}, {"x": 1, "color": "A"}, {"x": 0.91, "color": "B"}, {"x": 0.88, "color": "B"}, {"x": 0.12, "color": "A"} ] } }
Well spotted @mattijn thank you!
I'll follow this up in https://github.com/vega/altair/pull/3600#discussion_r1779673096
@mattijn I'm moving into draft to utilize https://github.com/vega/altair/pull/3633 and simplify https://github.com/vega/altair/pull/3600#discussion_r1781042923
Thanks for this PR. Looks good now!
Thanks for your help with this PR @mattijn
One last thing I'd like to check before merging, from the Deferred section of the description
tools.schemapi.vega_expr.py
->tools.vega_expr.py
- The module is unrelated to the
vega-lite
schema- I think the review will be simpler if it stays here for now
I can either:
vega_expr.py
in a follow-up PRvega_expr.py
now and then mergeOption 3 is OK 👍
Closes #3563
Description
This PR builds on the work of #3466, and provides accurate signatures for
alt.expr
methods.Benefits
Originally listed in https://github.com/vega/altair/issues/3563#issue-2494365499
TypeError
at the time ofexpr
definitionChartType
python
and notjavascript
expr
and the missing argument""
for theelseValue
Screenshots
Implementation
In #3563 I proposed authoring these changes manually. However, after revisiting a conversation I had with @jonmmease I thought it would be worth at least trying to generate the definitions.
I found that https://vega.github.io/vega/docs/expressions/ seemed to be the only place they are documented in a single place. From my understanding, this document is manually maintained but updates occur infrequently.
There was enough consistency in the markup used to work out how various parameters aligned with
python
syntax https://docs.python.org/3/tutorial/controlflow.html#special-parameters.I also used this opportunity to clean up the docs themselves, and get them closer to the rest of
altair
. There is definitely more that could be explored on that front, and some of the changes here could be utilised to improve the existing generated docs.Related
Tasks
py
-equivalent signaturesaltair
functionsbackticks
for own parametersmistune
escapingtest_expr.py
to account for number of arguments relatedalt.expr.__init__.py
.rst
stuff commitvega_expr.py
Deferred
Trying my best to keep the scope of the PR focused. Working on this sparked a lot of ideas, so I've collected those here to possibly explore in the future:
expressions.md
tools.schemapi.vega_expr.py
->tools.vega_expr.py
vega-lite
schemaexpr
methods, replace w/ snake_case_ExprMeta.__getattr__
would make this possible(Early) Preview
Prior to
5e75051
(#3600) there wasn't any generated source on this branch. Keeping these screenshots for a quick referenceuntil I've resolved some test issues.Screenshots
![image](https://github.com/user-attachments/assets/93a59695-c24a-4aba-ba9e-7a0b4bff11d2) ![image](https://github.com/user-attachments/assets/3282c09a-2c21-473c-a607-09b3c35d539f) ![image](https://github.com/user-attachments/assets/92e18f27-dc90-4209-9f8c-c9ccb2fb8673)