-
Notifications
You must be signed in to change notification settings - Fork 794
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
More convenient syntax instead of FieldOneOfPredicate #3239
Comments
I think that you would need a custom Altair list instead of using a python |
I was wondering about this. What made me think that it might be possible was that we do use the standard Python operators for the other comparisons, but I hear that this might not work the same for the |
I have a couple of alternative ideas for this one. 1 - A wrapper function (
|
Thanks for these great ideas @dangotbanned ! I think I agree that a submodule is better than top module to avoid cluttering the top level api and allow easy discovery of related functionality. However, of the existing field predicates, most are already covered by the regular comparison operators I wonder if we could be closer to how the syntax looks in Python (since this is what is used for the already existing comparison operators). I'm a bit short of time at the moment to dive deep into things, but do you think something along these lines would be possible or at they both susceptible to the constraints Mattijn mentioned above? # 1
alt.datum.Origin in alt.list('Japan', 'Europe')
alt.datum.Origin in alt.range(1, 10)
# 2
alt.datum.Origin alt.in ['Japan', 'Europe']
alt.datum.Origin alt.in range(1, 10) |
Thanks for the response @joelostblom
Yeah I agree. I've personally not used any of them before, so mostly just included them for completeness. Continuation of Utility class for all field predicates (field)One additional thing I thought of, but seemed a bit much for the initial idea. So this allows using # Previously ref as (2)
class field_old:
@classmethod
def one_of(
cls,
field: str,
/,
*oneOf: bool | float | Map | SchemaBase,
timeUnit= Undefined,
) -> FieldOneOfPredicate:
return FieldOneOfPredicate(field=field, oneOf=oneOf, timeUnit=timeUnit)
@classmethod
def range(
cls,
field: str,
range: Optional[
dict
| Parameter
| SchemaBase
| Sequence[dict | None | float | Parameter | SchemaBase]
] = Undefined,
/,
**kwds,
) -> FieldRangePredicate: ...
# ----------------------------------------------------
# Extending for composition (3)
class field:
@classmethod
def one_of(
cls,
field: str,
/,
*values: bool | float | dict[str, Any] | SchemaBase,
timeUnit: TimeUnitType = Undefined,
) -> SelectionPredicateComposition:
m = FieldOneOfPredicate(field=field, oneOf=values, timeUnit=timeUnit).to_dict()
return SelectionPredicateComposition(m)
@classmethod
def range(
cls, field: str, value: RangeType, /, *, timeUnit: TimeUnitType = Undefined
) -> SelectionPredicateComposition:
m = FieldRangePredicate(field=field, range=value, timeUnit=timeUnit).to_dict()
return SelectionPredicateComposition(m) I was curious on if this would actually render, and surpisingly it did!
Feedback on @joelostblom's idea
My first thoughts on
Since both
But apparently shadowing a builtin with a function isn't an issue? Personally, I think it would be error prone since a user could do this and accidentally change the meaning of their code: from altair import list, range
...
list(...)
range(...)
For
Also VSCode yelling at me:
Which is the same reason you'll see altair/altair/vegalite/v5/api.py Lines 1624 to 1628 in 9afd374
|
I quite like the |
Yup, definitely. We would have to append an underscore like we do with
I like this! Maybe just go ahead with the |
Thanks @mattijn
@joelostblom You can absolutely have both for Most of the
So for But I think for most cases it would be |
I saw #3440 (comment) again. import altair as alt
from vega_datasets import data
source = data.disasters()
columns_sorted = ['Drought', 'Epidemic', 'Earthquake', 'Flood']
alt.Chart(source, height=200).transform_filter(
{'and': [
alt.FieldOneOfPredicate(field='Entity', oneOf=columns_sorted), # Filter data to show only disasters in columns_sorted
alt.FieldRangePredicate(field='Year', range=[1900, 2000]) # Filter data to show only 20th century
]}
).mark_line(interpolate='monotone').encode(x=alt.X('Year:Q').axis(format='d'), color='Entity',column='Entity', y='sum(Deaths)') Will it be as such? alt.field.one_of(field='Entity', one_of=columns_sorted) & alt.field.range(field='Year', range=[1900, 200]) Also linking #695 here. Is this applicable here too? |
`field` proposed in vega#3239 (comment) `agg` was developed during vega#3427 (comment) as a solution to part of vega#3476
@mattijn I've opened #3505 to allow you (and anyone really) to test out what I've presented so far.
It would be the slightly shorter: # Also valid
# alt.field.one_of('Entity', 'Drought', 'Epidemic', 'Earthquake', 'Flood')
alt.field.one_of('Entity', columns_sorted) & alt.field.range('Year', [1900, 2000]) Each See #3239 (comment) for extra detail on that choice |
Writing a condition that matches a single value in relatively straightforward:
But when we want to match multiple values, the
alt.datum.Origin == 'Japan',
needs to be replaced with the notably more compllicated:It would be nice to support the
in
operator here and allow the syntax to just bealt.datum.Origin in ['Japan', 'Europe']
. I haven't looked into the change necessary to make this happen but noting it down as something to revisit in the future.The text was updated successfully, but these errors were encountered: