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

fix(python, rust): enforce sortedness of by argument in rolling_* functions #11002

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

MarcoGorelli
Copy link
Collaborator

closes #10991

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Sep 8, 2023
@orlp
Copy link
Collaborator

orlp commented Sep 8, 2023

I believe we shouldn't merge this, please see my concerns here: #10991.

@MarcoGorelli
Copy link
Collaborator Author

MarcoGorelli commented Sep 10, 2023

Sure, closing then


Actually, reopening

I agree with the concerns raised in #10991, and agree that the proposed solution (temporary sort) is better than this PR

But, if nobody has time* to implement the proposed solution from #10991 in time for 0.19.3 (which I presume will come out some time next week?), then I'd still like to suggest that this PR is better than the status-quo (which silently gives wrong results)

After all, upsample, group_by_rolling, and group_by_dynamic all already do what this PR proposes

*I'm travelling at the moment so don't really have time for anything non-trivial unfortunately

Polars downloads are exploding and I'm worried that someone might get wrong results and lose faith in the library

@orlp
Copy link
Collaborator

orlp commented Sep 11, 2023

I would not mind merging it as long as it's a temporary stopgap until we fix the feature to always work.

@ritchie46
Copy link
Member

Let's temporary require this flag. @MarcoGorelli can you rebase?

@MarcoGorelli
Copy link
Collaborator Author

First time I try rebasing from my phone whilst on a ferry and it seems to not have worked 😄 Will address this evening, and will make sure to remove the temporary stopgap early next month

@MarcoGorelli MarcoGorelli marked this pull request as draft September 12, 2023 08:52
@MarcoGorelli MarcoGorelli marked this pull request as ready for review September 12, 2023 19:08
@ritchie46 ritchie46 merged commit e88fded into pola-rs:main Sep 14, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rolling_* functions don't check sortedness
3 participants