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

Implement $rolling() for DataFrame and LazyFrame #682

Merged
merged 20 commits into from
Jan 10, 2024
Merged

Conversation

etiennebacher
Copy link
Collaborator

@etiennebacher etiennebacher commented Jan 8, 2024

No description provided.

@etiennebacher etiennebacher marked this pull request as ready for review January 10, 2024 09:23
Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.
I don't think this currently supports $ungroup(). Need to add tests.

Also, rather than making the implementation of RPolarsGroupBy complicated like this, I thought it would be simpler and better to delete by_agg on the Rust side and rewrite it to always call $lazy() and $collect() on the R side.

//used in GroupBy, not DataFrame
pub fn by_agg(
&mut self,
group_exprs: Robj,
agg_exprs: Robj,
maintain_order: Robj,
) -> RResult<RPolarsDataFrame> {
let group_exprs: Vec<pl::Expr> = robj_to!(VecPLExprCol, group_exprs)?;
let agg_exprs: Vec<pl::Expr> = robj_to!(VecPLExprColNamed, agg_exprs)?;
let maintain_order = robj_to!(Option, bool, maintain_order)?.unwrap_or(false);
let lazy_df = self.clone().0.lazy();
let lgb = if maintain_order {
lazy_df.group_by_stable(group_exprs)
} else {
lazy_df.group_by(group_exprs)
};
RPolarsLazyFrame(lgb.agg(agg_exprs)).collect()
}

@etiennebacher
Copy link
Collaborator Author

I thought it would be simpler and better to delete by_agg on the Rust side and rewrite it to always call $lazy() and $collect() on the R side.

I'll open a new PR for that as it touches the $agg() behavior overall, not just for rolling computations. Would it be ok for you to have a "dirty" implementation of $ungroup() similar to the change in $agg() here in the meantime?

@eitsupi
Copy link
Collaborator

eitsupi commented Jan 10, 2024

Would it be ok for you to have a "dirty" implementation of $ungroup() similar to the change in $agg() here in the meantime?

Of course it's okay. Thanks!

R/dataframe__frame.R Outdated Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
etiennebacher and others added 2 commits January 10, 2024 13:26
Co-authored-by: eitsupi <50911393+eitsupi@users.noreply.github.com>
@etiennebacher etiennebacher merged commit e457053 into main Jan 10, 2024
@etiennebacher etiennebacher deleted the df-lf-rolling branch January 10, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants