BUG: support corr and cov functions for custom BaseIndexer rolling windows #33804
Conversation
…olling operations (pandas-dev#33057)" This reverts commit 2bf54a8.
|
@jreback @mroeschke |
AlexKirko
changed the title
| if isinstance(self.window, BaseIndexer): | ||
| window = self.window | ||
| else: | ||
| window = self._get_window(other) if not self.is_freq_type else self.win_freq |
There was a problem hiding this comment.
can you add a comment here and on cov explaining what e are doing (generally not specific to BaseIndexer)
There was a problem hiding this comment.
@jreback Done. Please take a look if it's what you had in mind.
| if self.is_freq_type: | ||
| window = self.win_freq | ||
| if isinstance(self.window, BaseIndexer): | ||
| window = self.window |
| if isinstance(self.window, BaseIndexer): | ||
| window = self.window | ||
| else: | ||
| window = self._get_window(other) if not self.is_freq_type else self.win_freq |
There was a problem hiding this comment.
can you add a comment here and on cov explaining what e are doing (generally not specific to BaseIndexer)
|
Looks like you've also committed a |
@mroeschke |
|
CI is being uncooperative. Probably temporary, will restart later. |
|
All green, please take a look at the post where I recorded the results of digging into |
|
thanks @AlexKirko very nice work on this series! keep em coming! |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffScope of PR
This is the final PR needed to solve #32865. It fixes
covandcorrby passing the correct argument torollingconstructors if the functions are called with a customBaseIndexersubclass. A separate test is added forcovandcorras the previous tests were single-variable and wouldn't work with two columns of data. I also propose we revert #33057.Details
The reason
covandcorrdidn't work was that we were passing_get_windowresults (usually, window width), instead of theBaseIndexersubclass torollingconstructors called inside the function. Passingself.window(when necessary) fixes the issue.The new test asserts only against an explicit array, because shoehorning a comparison against
np.covannp.corrcoefis ugly (I've tried). Ourrolling.applyisn't really suited to be called with bivariate statistics functions, and the necessary gymnastics didn't look good to me. I don't believe the minor benefit would be worth, but please say if you think coomparing against numpy is necessary.Thanks to @mroeschke for implementing the
NotImplementederror-raising behavior in #33057 . Now that all the functions are fixed, I propose we revert that commit.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 algorithms.