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

feat(RFC): Adds agg, field utility classes #3505

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jul 26, 2024

Will close #3239

Description

I've isolated these into a new module.
Currently, this is only to support discussions like #3239 (comment) to test out the syntax/design.

Status

I'm not looking to add these into api.py until these are resolved:

They will be too difficult to find currently, but having this in a public branch will make refining the design easier.

Related Issues

#1642 can be solved by accepting str | Mapping | None here:
https://github.com/dangotbanned/altair/blob/0e806ce55db1acd7e8591c7eb0c30101baf26d2b/altair/vegalite/v5/_api_rfc.py#L147-L157

Purely for visual demonstration
Ensures the same output from
```py
alt.Y("count()")
alt.Y(alt.agg.count())
```
SelectionPredicateComposition({'field': 'Origin', 'oneOf': ['Japan', 'Europe']})
"""

def __new__( # type: ignore[misc]
Copy link
Member Author

@dangotbanned dangotbanned Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to highlight this current design quirk.

import altair as alt

>>> alt.field("Origin:N")
{'field': 'Origin', 'type': 'nominal'}

>>> alt.agg("Origin:N")
{'field': 'Origin', 'type': 'nominal'}
  1. I'm anticipating users would expect alt.field() to work this way, despite shorthand being unrelated.
  2. I also think using alt.agg() without specifying an aggregation wouldn't seem an intuitive way to define a field.
  3. The other choices for what to return
    • Each wrapped Field...Predicate already has a constructor
    • the faux-parent Predicate class is just a Union
    • the unrelated Field class seems to be wrapping a str (not a {"field": "..."})

@dangotbanned dangotbanned added the v6 Aim to be completed for version 6 label Aug 13, 2024
- When I originally wrote this, not all of these were available
- Overall, trying to reduce verbosity
Simply rewrites as `FieldValidPredicate("field", valid=False)`
> If set to true the field's value has to be valid, meaning both not ``null`` and not NaN
@dangotbanned dangotbanned linked an issue Dec 23, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement v6 Aim to be completed for version 6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More convenient syntax instead of FieldOneOfPredicate Improve syntax for argmin/max
2 participants