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

Add min periods parameter to Window #1792

Merged
merged 7 commits into from May 24, 2019

Conversation

Projects
3 participants
@emilyreff7
Copy link
Contributor

commented May 23, 2019

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.

@cpcloud cpcloud added this to the 1.1.0 milestone May 23, 2019

@cpcloud cpcloud added this to To do in Pandas via automation May 23, 2019

@cpcloud cpcloud self-requested a review May 23, 2019

@cpcloud

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@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 pep8speaks to ibis so that these kinds of failures are immediately surfaced rather than having to hunt through CI.

You can also see these failures on the command line by running make lint.

@cpcloud

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@emilyreff7 Another thing that will prevent lint issues in a PR is to setup the commit hooks using pre-commit. Once you've created your environment you can run pip install pre-commit and then inside your ibis clone run pre-commit install. Then when you run git commit the lint and style checks will run and will prevent committing unless they pass.

@cpcloud
Copy link
Member

left a comment

Looks good, just a couple of comments.

Show resolved Hide resolved ibis/pandas/tests/test_udf.py Outdated
Show resolved Hide resolved ibis/pandas/aggcontext.py Outdated
@emilyreff7

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@emilyreff7 Another thing that will prevent lint issues in a PR is to setup the commit hooks using pre-commit. Once you've created your environment you can run pip install pre-commit and then inside your ibis clone run pre-commit install. Then when you run git commit the lint and style checks will run and will prevent committing unless they pass.

I ran the above but got an error once I got to the git commit step:

Errors:
Traceback (most recent call last):
File "/home/emily/anaconda3/envs/ibis-dev/lib/python3.6/site-packages/virtualenv.py", line 2580, in
main()
File "/home/emily/anaconda3/envs/ibis-dev/lib/python3.6/site-packages/virtualenv.py", line 831, in main
symlink=options.symlink,
File "/home/emily/anaconda3/envs/ibis-dev/lib/python3.6/site-packages/virtualenv.py", line 1123, in create_environment
install_wheel(to_install, py_executable, search_dirs, download=download)
File "/home/emily/anaconda3/envs/ibis-dev/lib/python3.6/site-packages/virtualenv.py", line 973, in install_wheel
_install_wheel_with_search_dir(download, project_names, py_executable, search_dirs)
File "/home/emily/anaconda3/envs/ibis-dev/lib/python3.6/site-packages/virtualenv.py", line 1060, in _install_wheel_with_search_dir
call_subprocess(cmd, show_stdout=False, extra_env=env, stdin=script)
File "/home/emily/anaconda3/envs/ibis-dev/lib/python3.6/site-packages/virtualenv.py", line 924, in call_subprocess
raise OSError("Command {} failed with error code {}".format(cmd_desc, proc.returncode))
OSError: Command /home/emily/.cache/p...hon3.6/bin/python3.6 - setuptools pip wheel failed with error code 2

I re-installed pre-commit using conda (conta install -c conda-forge pre_commit) and it resolved the issue.

Also now just waiting on the checks to run...have been running/queued up for 1+ hours now.

@cpcloud

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@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 ibis-project user. I can help you set that up if you want.

@cpcloud

This comment has been minimized.

Copy link
Member

commented May 24, 2019

In any event, this LGTM.

@cpcloud cpcloud merged commit d619ca8 into ibis-project:master May 24, 2019

13 checks passed

ci/circleci: python35_test Your tests passed on CircleCI!
Details
ci/circleci: python36_benchmark Your tests passed on CircleCI!
Details
ci/circleci: python36_conda_build Your tests passed on CircleCI!
Details
ci/circleci: python36_docs Your tests passed on CircleCI!
Details
ci/circleci: python36_test Your tests passed on CircleCI!
Details
ci/circleci: python37_conda_build Your tests passed on CircleCI!
Details
ci/circleci: python37_test Your tests passed on CircleCI!
Details
ibis-project.ibis Build #20190523.10 succeeded
Details
ibis-project.ibis (WindowsCondaBuild py36) WindowsCondaBuild py36 succeeded
Details
ibis-project.ibis (WindowsCondaBuild py37) WindowsCondaBuild py37 succeeded
Details
ibis-project.ibis (WindowsTest py35) WindowsTest py35 succeeded
Details
ibis-project.ibis (WindowsTest py36) WindowsTest py36 succeeded
Details
ibis-project.ibis (WindowsTest py37) WindowsTest py37 succeeded
Details

Pandas automation moved this from To do to Done May 24, 2019

@emilyreff7 emilyreff7 deleted the emilyreff7:rolling branch May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.