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

Improve expr method signatures #3563

Closed
dangotbanned opened this issue Aug 29, 2024 · 5 comments · Fixed by #3600
Closed

Improve expr method signatures #3563

dangotbanned opened this issue Aug 29, 2024 · 5 comments · Fixed by #3600

Comments

@dangotbanned
Copy link
Member

dangotbanned commented Aug 29, 2024

Problem

While I was working on #3544, I tried out re-writing some expression strings to use alt.expr.
This idea stems from what I saw as a poor UX - that has a relatively simple solution with multiple benefits.

Related

Minimal Repro

Based on a681ec5

Code block

import altair as alt

label = alt.datum.label
window_lead_label = alt.datum.window_lead_label

source = {
    "values": [
        {"label": "Begin", "amount": 4000},
        {"label": "Jan", "amount": 1707},
        {"label": "Feb", "amount": -1425},
        {"label": "Mar", "amount": -1030},
        {"label": "Apr", "amount": 1812},
        {"label": "May", "amount": -1067},
        {"label": "Jun", "amount": -1481},
        {"label": "Jul", "amount": 1228},
        {"label": "Aug", "amount": 1176},
        {"label": "Sep", "amount": 1146},
        {"label": "Oct", "amount": 1205},
        {"label": "Nov", "amount": -1388},
        {"label": "Dec", "amount": 1492},
        {"label": "End", "amount": 0},
    ]
}

chart = (
    alt.Chart(source)
    .mark_bar(size=45)
    .encode(alt.X("label:O", sort=None))
    .transform_window(window_lead_label="lead(label)")
    .transform_calculate(calc_lead=alt.expr.if_(window_lead_label == None, label))
)
>>> alt.expr.if_(window_lead_label == None, label)
>>> chart

Visual Feedback

Screenshot

image

  • Error is only present after validation
  • The error itself is not useful if the spec contains more than one expr
  • Also not clear which argument(s) are missing

Solution

Replacing the alt.expr classmethod(s) *args parameter with named positional-only parameter(s).

Benefits

  • Raising a TypeError at the time of expr definition
    • Rather than silently passing until validated within a ChartType
    • Importantly, this is within python and not javascript
  • Providing a meaningful traceback, identifying both the expr and the missing argument
  • Opens the door for defining default argument values
    • E.g. in waterfall_chart, there are repeat uses of "" for the elseValue
    • If this is a common case, we could use that as a default and it would be clear by the signature alone
  • The current docstrings - which refer to parameters by name - would be easier to understand
    • Right now, you'd need to refer to Vega Expressions to determine the order they must appear in
  • The change would not add any new requirements to currently valid specs
    • Therefore, suitable as a MINOR version feature

Example (alt.expr.if_)

Although there are a large number of signatures to be updated, the process for each should be as simple as refering to the docstring or Vega Expressions

image

Drawbacks

The only downside I can see - hypothetically - is if there is a utility in having invalid expressions at expr definition time?
Maybe there is some use-case here I'm unaware of, but it seems unlikely to me it could outweigh the traceback improvement

@binste
Copy link
Contributor

binste commented Sep 4, 2024

I like it! Sounds like a big UX improvement for the Python-side of the expression syntax. And I also don't see any drawback to this, defining invalid expressions could still be done by just skipping the expr functionalities and providing strings (just in case anyone would need this which I also don't expect).

@joelostblom
Copy link
Contributor

I also think it would be great to have better error messages from expressions, thanks for exploring this! Directly to your comment about using alt.expr in the docs instead of expression strings: I think we would actually like to do this everywhere once the full functionality of expression strings is present in alt.expr (the only one I'm aware is missing currently is #3366). The changes you suggest here are a further improvement and makes the case even stronger for switching to alt.expr everywhere.

@dangotbanned
Copy link
Member Author

@mattijn @jonmmease do you guys have any thoughts on this idea?

(providing everyone was onboard)
I think we could implement this quite quickly; especially if anyone else wanted to help spread the workload out 🤝

@mattijn
Copy link
Contributor

mattijn commented Sep 6, 2024

I believe the Vega Expressions syntax is a powerful asset of the grammar that remains under-utilised. I'm very supportive with any attempt to improve it, such as your suggestion here to introduce positional-only arguments.

However, if a broader overhaul is needed to improve the ergonomics, making it more accessible and pythonic, I am also in favour. And to improve this properly I think this requires a major version bump, and it would be great to have one. So yeah, good idea, but if your brain can see further, also great!😊

@binste
Copy link
Contributor

binste commented Sep 6, 2024

Exciting! I like the idea of trying to get to a v6. That would allow us to work on these: https://github.com/vega/altair/issues?q=is%3Aopen+is%3Aissue+label%3Av6 Especially, #2918 could be of big help for users.

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

Successfully merging a pull request may close this issue.

4 participants