Skip to content

Improve expr method signatures #3563

Closed

Description

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

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

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions