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: Raise informative error message if non-IntoExpr is passed by name in *Frame.group_by #17654

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

MarcoGorelli
Copy link
Collaborator

@MarcoGorelli MarcoGorelli commented Jul 16, 2024

closes #17540

Although the current behaviour is technically correct, I think this is worth doing - the pandas docs have examples such as

df.groupby(by=["b"], dropna=False).sum()

so the many pandas users trying out Polars would now get an informative error

I did debate whether this should raise a warning instead of an error, but my


Demo:

import polars as pl

df = pl.DataFrame({"a": [1, 1, 2], "b": [4, 5, 6]})

print(df.group_by(by=["a"]).agg([pl.col("b").sum()]))
TypeError: Expected Polars expression or object convertible to one, got <class 'list'>.

Hint: if you tried
    group_by(by=['a'])
then you probably want to use this instead:
    group_by(['a'])

I did debate whether it would be better to raise a warning instead...however, I really can't think of a use case for passing a list literal to group_by, it defeats the purpose of the operation

@MarcoGorelli MarcoGorelli changed the title feat: raise informative error message if list or tuple is passed to by in *Frame.group_by feat: raise informative error message if non-IntoExpr is passed by name in *Frame.group_by Jul 16, 2024
@MarcoGorelli MarcoGorelli changed the title feat: raise informative error message if non-IntoExpr is passed by name in *Frame.group_by feat: Raise informative error message if non-IntoExpr is passed by name in *Frame.group_by Jul 16, 2024
@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars and removed title needs formatting labels Jul 16, 2024
Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.71%. Comparing base (30bf890) to head (ef3fe8d).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #17654   +/-   ##
=======================================
  Coverage   80.70%   80.71%           
=======================================
  Files        1485     1485           
  Lines      195511   195535   +24     
  Branches     2782     2786    +4     
=======================================
+ Hits       157788   157824   +36     
+ Misses      37211    37199   -12     
  Partials      512      512           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MarcoGorelli MarcoGorelli marked this pull request as ready for review July 16, 2024 09:35
@MarcoGorelli MarcoGorelli force-pushed the positional-group-by-errmsg branch 2 times, most recently from 02dec51 to 6a1cbbe Compare July 16, 2024 09:40
@ritchie46 ritchie46 merged commit ecddc13 into pola-rs:main Jul 16, 2024
15 checks passed
@tylerriccio33
Copy link
Contributor

@MarcoGorelli I was looking into this issue last night and thought the GroupBy class could have it's type hints switched to something other than IntoExpr. The IntoExpr includes List[Any] (part of PythonLiteral), which is no longer allowed. Should there be a new type like GroupByIntoExpr?

The type could look something like this (below) which more accurately represents what input should go into the keyword arguments.

GroupByIntoExpr: TypeAlias = Union[NonNestedLiteral, IntoExprColumn, None]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise informative error message when group_by(by=[...]) is used
3 participants