-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
Center window #36097
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
Center window #36097
Conversation
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.
needs a fair amount of tests
@@ -76,14 +79,27 @@ def calculate_variable_window_bounds( | |||
# right endpoint is open | |||
else: | |||
end[0] = 0 | |||
if center_window: | |||
for j in range(0, num_values+1): | |||
if (index[j] == index[0] + index_growth_sign*window_size/2 and |
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.
this is likley failing linting (need spaces between operators)
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
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.
@adamamiller are you interested in continuing? If yes please address @jreback comment (he's requesting tests) and merge master/resolve conflicts
Closing for now. @adamamiller ping whenever you'd like to continue and we'll reopen! |
Would love to see this functionality and would like to pick up the pull request with @luholgit What are the next required steps? We already forked the pandas repo and checked out the branch from @adamamiller. However, when trying to update the branch by pulling from origin/master into fork/center_window we encountered several merge conflicts which we are not sure how to solve. Should we solve them based on our understanding and then discuss those changes in this PR? |
you see the comments above this needs to have master merged and needs tests |
Thanks for your quick response, we are implementing the merge and tests right now. (Un)Fortunately, we were not able to push to @adamamiller's fork. Could you change the pull request to come from https://github.com/luholgit/pandas/tree/center_window? |
Or shall we create a new issue such that the new source branch from our fork is selected automatically? |
@Zaubeerer thanks for taking this forwards - you can open a new pull request |
@Zaubeerer you just create a new PR, no need to create a new issue |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Added center functionality for
VariableWindowIndexer
. Note - I am unsure if the NotImplementedError in lines 1966-1969 in rolling.py still correctly raises an error for offset based windows.