vega / altair

Declarative statistical visualization library for Python
https://altair-viz.github.io/
BSD 3-Clause "New" or "Revised" License
9.35k stars 794 forks source link

The parameter `empty` can be used in multiple places which lead to different error messages #3596

Open joelostblom opened 1 month ago

joelostblom commented 1 month ago

What happened?

I was playing around with this gallery example when I noticed that there seems to be multiple ways of specifying empty=True inside when-then-otherwise. The default in the example is to put it inside alt.value:

alt.when(select).then(alt.value(2, empty=False))

However, it also works to put it inside then():

alt.when(select).then(alt.value(2), empty=False)

and inside when():

alt.when(select, empty=False).then(alt.value(2))

They all work they same, but somewhat confusingly, the addition of other parameters leads to different behavior between the three. For when no error is raised but the parameter is also not applied, e.g.:

alt.when(select, empty=False, nearest=True)

For the other two, an error is raised, but it points the user in the wrong direction:

alt.when(select).then(alt.value(2), empty=False, nearest=True)
SchemaValidationError: `StrokeWidth` has no parameter named 'empty'

What would you like to happen instead?

I think the ideal case would be if we did not allow for empty in then() and instead only in when which is also clearly documented currently.

I don't think there is anything we can do about value since it has always accepted kwds. But if the error message could be made clearer, then that would be helpful (maybe related to https://github.com/vega/altair/issues/2913). I think the clearest possible message in the case of

alt.when(select).then(alt.value(2), empty=False, nearest=True)

would be

SchemaValidationError: `When` has no parameter named 'nearest'

cc @dangotbanned since you have the most expertise here, do you think it is possible to raise this error instead of the current behavior?

Which version of Altair are you using?

5.4.1

dangotbanned commented 1 month ago

@joelostblom two points that stood out to me:

  1. You mention empty, but the last three code blocks also contain nearest=True.
    • Is that required for the repro?
    • Also could you add the definition of select in
      • alt.when(select).then(alt.value(2, empty=False))
  2. In #3567, I've improved the docs, typing and added a new exception case.
joelostblom commented 1 month ago

The code is from this gallery example so select is:

select = alt.selection_point(name="select", on="click")

Without nearest there is no error message. But with nearest alone there is an error message if it is passed to value, but not if it passed to when or then:

alt.when(select).then(alt.value(2, nearest=False))
SchemaValidationError: `StrokeWidth` has no parameter named 'nearest'

This error makes sense since the kwds of value are applied to strokeWidth, but it doesn't make sense that when using alt.value(2, empty=False)) the keyword empty is passed to the param; it should also raise an error since strokeWidth does not have a kwd called empty.

Could you try this on that branch please?

Will try that out later today!

dangotbanned commented 1 month ago
Previous comment > The code is from [this gallery example](https://altair-viz.github.io/gallery/interactive_bar_select_highlight.html#gallery-interactive-bar-select-highlight) so `select` is: > > ```python > select = alt.selection_point(name="select", on="click") > ``` > > Without `nearest` there is no error message. But with `nearest` alone there is an error message if it is passed to `value`, but not if it passed to `when` or `then`: > > ```python > alt.when(select).then(alt.value(2, nearest=False)) > ``` > > ``` > SchemaValidationError: `StrokeWidth` has no parameter named 'nearest' > ``` > > This error makes sense since the `kwds` of value are applied to `strokeWidth`, but it doesn't make sense that when using `alt.value(2, empty=False))` the keyword `empty` is passed to the param; it should also raise an error since `strokeWidth` does not have a kwd called `empty`. > > > Could you try this on that branch please? > > Will try that out later today!

Okay I think I see the confusion @joelostblom:

Repro

import altair as alt

select = alt.selection_point(name="select", on="click")
highlight = alt.selection_point(name="highlight", on="pointerover", empty=False)
stroke_width = (
    alt.when(select)
    .then(alt.value(2, empty=False))
    .when(highlight)
    .then(alt.value(1))
    .otherwise(alt.value(0))
)

then_nearest_1 = alt.when(select).then(alt.value(2, nearest=False)).to_dict()
# Just testing `alt.value` isn't contributing to the issue
then_nearest_2 = alt.when(select).then(alt.value(2), nearest=False).to_dict()

print(stroke_width, then_nearest_1, then_nearest_2, sep="\n")

Output

# -------------------------------------------->
{'condition': [{'param': 'select', 'value': 2, 'empty': False}, {'param': 'highlight', 'empty': False, 'value': 1}], 'value': 0}
{'condition': [{'param': 'select', 'value': 2, 'nearest': False}]}
{'condition': [{'param': 'select', 'value': 2, 'nearest': False}]}

I had already added links to this page on #3567 but empty is valid (as is value) in each of those pairs.

So each element in the list keyed to {"condition": [...]} should look like this:

Code block

```py class _ConditionClosed(TypedDict, closed=True, total=False): # type: ignore[call-arg] # https://peps.python.org/pep-0728/ # Parameter {"param", "value", "empty"} # Predicate {"test", "value"} empty: Optional[bool] param: Parameter | str test: _TestPredicateType value: Any _Conditions: TypeAlias = t.List[_ConditionClosed] """ Chainable conditions produced by ``.when()`` and ``Then.when()``. All must be a `Conditional Value`_. .. _Conditional Value: https://vega.github.io/vega-lite/docs/condition.html#value """ ```