Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BUG: support min/max functions for rolling windows with custom BaseIndexer #33180
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
BUG: support min/max functions for rolling windows with custom BaseIndexer #33180
Changes from 12 commits
722710d
c8d79f0
53947cd
6c77427
312e921
3b2067d
ede2da7
d91fb10
f52fcff
7a5a49d
723713e
560f7df
1c394e7
4e9772d
cc0b56e
c5ab8bc
5204a6c
72970b7
b9ddfc1
9cfea82
82fe546
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
hmm are you anticpating exposing this to users? should we be using this internally?
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.
Do you mean allowing the creation of a forward-looking rolling window without creating a custom
BaseIndexer
subclass?My goal was to make sure that our implementations are robust against custom indexers with arbitrary offsets relative to the position in the input array they are calculated for. Additionally, I made this implementation robust against weird stuff, like smoothly expanding or contracting windows, for example.
The
ForwardIndexer
class is used in testing for convenience, because it is, by far, the most common customization people expect us to support, in my limited experience.We could make forward-looking a Boolean argument in the rolling window constructor, but I don't know how useful of an enhancement it would be. In my opinion, it's enough to make sure that we support reasonable customizations based on
BaseIndexer
and let the users take care of the rest.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.
Might be interesting to provide a standard set of
BaseIndexer
s in the library.ForwardIndexer
may be one that we provide once we standardize the definition and API.@AlexKirko feel free to open a new issue about including
ForwardIndexer
andBackwardIndexer
in thepandas.api.indexers
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.
Thanks, created a new issue. Will be happy to submit a PR in a day or two.
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.
Can we also test this against
rolling.apply(lambda x: max(x))
(or the equivalent)?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.
Done.
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.
@mroeschke
Should we drop testing against an explicitly provided array then? Doing both seems redundant to me.
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.
I think it's okay to keep the explicit array as the source of truth on the off chance that
rolling(Indexer).max()
androlling.apply(lambda x: max(x)
both incorrectly change in the same directionThere 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.
Okay, let's keep it then. I was afraid of something like this, which is why I started by testing against an explicit array and not against apply.