BUG: support count function for custom BaseIndexer rolling windows #33605
Conversation
BaseIndexer support validation is covered by the validation in _get_window_indexer
| if self.is_freq_type: | ||
| # GH 32865. Also use custom rolling windows calculation | ||
| # when using a BaseIndexer subclass | ||
| if self.is_freq_type or isinstance(self.window, BaseIndexer): |
There was a problem hiding this comment.
This is the crux of the issue. Because we didn't check for BaseIndexer subclass here, the algorithm went down the default path, and the BaseIndexer implementation got completely ignored as blocks were created using _Window.create_blocks.
| @@ -1171,8 +1171,6 @@ class _Rolling_and_Expanding(_Rolling): | |||
| ) | |||
|
|
|||
| def count(self): | |||
| if isinstance(self.window, BaseIndexer): | |||
| validate_baseindexer_support("count") | |||
There was a problem hiding this comment.
Custom BaseIndexer implementations should never go into this function. Instead, _get_window_indexer gets called, and there is a validate_baseindexer_support there (checked that it works by temporarily blacklisting count again)
Removing this check as unnecessary.
There was a problem hiding this comment.
can u add an assertion here
|
@mroeschke @jreback All green, details above. |
|
@jreback |
|
thanks @AlexKirko |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffScope of PR
This PR makes sure that when we call
countwith aBaseIndexersubclass, customstartandendarrays get calculated, and the algorithm in theaggregations.pyxgets called instead of the_Window.create_blocks.Turns out that we were sending the calculation into
aggregations.pyxonly for frequency-based windows, and customBaseIndexersubclasses got ignored. Fixed it and cleaned the code up a bit.Background on the wider issue
We currently don't support several rolling window functions when building a rolling window object using a custom class descended from
pandas.api.indexers.Baseindexer. The implementations were written with backward-looking windows in mind, and this led to these functions breaking.Currently, using these functions returns a
NotImplementederror thanks to #33057, but ideally we want to update the implementations, so that they will work without a performance hit. This is what I aim to do over a series of PRs.Perf notes
No changes to the algorithms necessary.