Skip to content

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

Closed
wants to merge 3 commits into from
Closed

Conversation

adamamiller
Copy link

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.

@jreback jreback added the Window rolling, ewma, expanding label Sep 5, 2020
Copy link
Contributor

@jreback jreback left a 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
Copy link
Contributor

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)

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2020

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.

@github-actions github-actions bot added the Stale label Oct 6, 2020
Copy link
Member

@arw2019 arw2019 left a 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

@arw2019
Copy link
Member

arw2019 commented Nov 4, 2020

Closing for now. @adamamiller ping whenever you'd like to continue and we'll reopen!

@arw2019 arw2019 closed this Nov 4, 2020
@Zaubeerer
Copy link

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?

@Zaubeerer
Copy link

@jreback and @arw2019, mentioning you explicitly as we need this change urgently and would love to implement at the open source directly as opposed to our private repository! :)

@jreback
Copy link
Contributor

jreback commented Dec 29, 2020

you see the comments above

this needs to have master merged and needs tests

@Zaubeerer
Copy link

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?

@Zaubeerer
Copy link

Or shall we create a new issue such that the new source branch from our fork is selected automatically?

@MarcoGorelli
Copy link
Member

@Zaubeerer thanks for taking this forwards - you can open a new pull request

@jreback
Copy link
Contributor

jreback commented Dec 29, 2020

@Zaubeerer you just create a new PR, no need to create a new issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Window rolling, ewma, expanding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Center rolling window with date NotImplementedError
5 participants