-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add min periods parameter to Window #1792
Conversation
…lling Updating feature branch to current head.
@emilyreff7 It looks like you have a lint error: https://circleci.com/gh/ibis-project/ibis/7540?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link I'm going to looking into adding You can also see these failures on the command line by running |
@emilyreff7 Another thing that will prevent lint issues in a PR is to setup the commit hooks using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of comments.
I ran the above but got an error once I got to the Errors: I re-installed pre-commit using conda ( Also now just waiting on the checks to run...have been running/queued up for 1+ hours now. |
@emilyreff7 Sometimes the checks take awhile if you haven't enabled the CI services on your fork. If you enable CircleCI and Azure Pipelines on your fork then the CI will run with a separate set of containers than the ones that are used to run CI for the |
In any event, this LGTM. |
We want to set min_periods=1 on rolling windows to avoid defaulting to min_periods equal to the window size n, thereby eliminating the necessity of NaNs in the first n-1 rows for trailing window operations.