feat: support std and var with ddof !=1 in pandas-like group by #1645
Conversation
narwhals/_pandas_like/group_by.py
Outdated
| # Invert the dict to have root_name: output_name | ||
| # TODO(FBruzzesi): Account for duplicates | ||
| columns={v: k for k, v in output_to_root_name_mapping.items()}, |
There was a problem hiding this comment.
🤔 in case someone does .agg(nw.col('a').std(ddof=1).alias('b'), nw.col('a').std(ddof=1).alias('c'))? hmm yeah i guess it's possible
btw, since cuDF have no introduced cudf.NamedAgg, we could use that if it makes all this logic a bit easier
There was a problem hiding this comment.
Yes correct, similarly to simple aggs.
To avoid an explosion in the number of lists to keep track of, maybe there is a better data structure. I will think about it a bit, yet aside that specific case, this PR should be ready
narwhals/_pandas_like/group_by.py
Outdated
| [ | ||
| grouped[std_root_names] | ||
| .std(ddof=ddof) | ||
| .set_axis(std_output_names, axis="columns", copy=False) |
There was a problem hiding this comment.
@MarcoGorelli how bad is it to use set_axis to rename the columns? What's the alternative? Our rename does a mapping, yet here we could do:
grouped[["b", "b"]].std(ddof=2).set_axis(["c", "d"], axis="columns")For once we are exploiting some pandas weirdness 😁
There was a problem hiding this comment.
Should be fine let's just use copy=false
FBruzzesi
changed the title
|
oh nice |
What type of PR is this? (check all applicable)
Related issues
group_bycontext ignores expr arguments #1606Checklist
If you have comments or can explain your changes, please do so below