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

API: reimplement FixedWindowIndexer.get_window_bounds #37035

Merged
merged 6 commits into from
Oct 10, 2020

Conversation

justinessert
Copy link
Contributor

@justinessert justinessert commented Oct 10, 2020

This PR replaces 36132 following @mroeschke's 36567.

Originally, my PR was to fix Issue 36040. But it appears that Matthew's PR already fixed it! Nonetheless, I still included two things from my previous PR:

  1. I added tests to cover the bug outlined in Issue 36040.
  2. I reimplemented FixedWindowIndexer.get_window_bounds in a way that I believe if a lot more intuitive and simple. Please lmk if you disagree, in which case I could remove this from the PR.

I don't believe any functionality was altered by this PR so I did not include a whatsnew entry

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.

looks good @justinessert

pandas/core/window/indexers.py Outdated Show resolved Hide resolved
pandas/tests/window/test_grouper.py Show resolved Hide resolved
@jreback jreback added this to the 1.2 milestone Oct 10, 2020
@jreback
Copy link
Contributor

jreback commented Oct 10, 2020

cc @mroeschke

@mroeschke
Copy link
Member

Could you also add a whatsnew entry in 1.2?

@justinessert
Copy link
Contributor Author

Looks like there were some failures, but I do not believe those were caused by this PR

@justinessert
Copy link
Contributor Author

Could you also add a whatsnew entry in 1.2?

@mroeschke I added a whatsnew entry about the reimplementation and linked Issue 36040, even though it's not technically fixing that issue (the issue was already fixed by your PR). Please lmk if this is what you intended I add, or if you were looking for something else.

@jreback jreback merged commit 81e47fd into pandas-dev:master Oct 10, 2020
@jreback
Copy link
Contributor

jreback commented Oct 10, 2020

thanks @justinessert

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Groupby Window rolling, ewma, expanding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Rolling min_periods not working on groupby object
3 participants