Skip to content

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

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
Contributor

@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
Contributor

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