Skip to content

BUG: support count function for custom BaseIndexer rolling windows #33605

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Apr 17, 2020
Merged

BUG: support count function for custom BaseIndexer rolling windows #33605

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
98c7183
enable count for BaseIndexer windows
AlexKirko Apr 17, 2020
a812a8f
TST: add count to the test
AlexKirko Apr 17, 2020
b540cf0
ENH: actually get custom windows for count
AlexKirko Apr 17, 2020
0c212b8
remove validation from count
AlexKirko Apr 17, 2020
5877141
DOC: add whatsnew entry
AlexKirko Apr 17, 2020
af41d6f
DOC: clarify the comment
AlexKirko Apr 17, 2020
f8a126a
add self.window class check to count
AlexKirko Apr 17, 2020
12fc3b3
DOC: add comment to assert
AlexKirko Apr 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions doc/source/whatsnew/v1.1.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,8 @@ Other API changes
- Added :meth:`DataFrame.value_counts` (:issue:`5377`)
- :meth:`Groupby.groups` now returns an abbreviated representation when called on large dataframes (:issue:`1135`)
- ``loc`` lookups with an object-dtype :class:`Index` and an integer key will now raise ``KeyError`` instead of ``TypeError`` when key is missing (:issue:`31905`)
- Using a :func:`pandas.api.indexers.BaseIndexer` with ``count``, ``skew``, ``cov``, ``corr`` will now raise a ``NotImplementedError`` (:issue:`32865`)
- Using a :func:`pandas.api.indexers.BaseIndexer` with ``min``, ``max`` will now return correct results for any monotonic :func:`pandas.api.indexers.BaseIndexer` descendant (:issue:`32865`)
- Using a :func:`pandas.api.indexers.BaseIndexer` with ``skew``, ``cov``, ``corr`` will now raise a ``NotImplementedError`` (:issue:`32865`)
- Using a :func:`pandas.api.indexers.BaseIndexer` with ``count``, ``min``, ``max`` will now return correct results for any monotonic :func:`pandas.api.indexers.BaseIndexer` descendant (:issue:`32865`)
- Added a :func:`pandas.api.indexers.FixedForwardWindowIndexer` class to support forward-looking windows during ``rolling`` operations.
-

Expand Down
1 change: 1 addition & 0 deletions pandas/core/window/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ def func(arg, window, min_periods=None):
def validate_baseindexer_support(func_name: Optional[str]) -> None:
# GH 32865: These functions work correctly with a BaseIndexer subclass
BASEINDEXER_WHITELIST = {
"count",
"min",
"max",
"mean",
Expand Down
6 changes: 3 additions & 3 deletions pandas/core/window/rolling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,8 +1171,6 @@ class _Rolling_and_Expanding(_Rolling):
)

def count(self):
if isinstance(self.window, BaseIndexer):
validate_baseindexer_support("count")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can u add an assertion here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


blocks, obj = self._create_blocks()
results = []
Expand Down Expand Up @@ -1939,7 +1937,9 @@ def aggregate(self, func, *args, **kwargs):
def count(self):

# different impl for freq counting
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):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

window_func = self._get_roll_func("roll_count")
return self._apply(window_func, center=self.center, name="count")

Expand Down
3 changes: 2 additions & 1 deletion pandas/tests/window/test_base_indexer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def get_window_bounds(self, num_values, min_periods, center, closed):
df.rolling(indexer, win_type="boxcar")


@pytest.mark.parametrize("func", ["count", "skew", "cov", "corr"])
@pytest.mark.parametrize("func", ["skew", "cov", "corr"])
def test_notimplemented_functions(func):
# GH 32865
class CustomIndexer(BaseIndexer):
Expand All @@ -99,6 +99,7 @@ def get_window_bounds(self, num_values, min_periods, center, closed):
@pytest.mark.parametrize(
"func,np_func,expected,np_kwargs",
[
("count", len, [3.0, 3.0, 3.0, 3.0, 3.0, 3.0, 3.0, 3.0, 2.0, np.nan], {},),
("min", np.min, [0.0, 1.0, 2.0, 3.0, 4.0, 6.0, 6.0, 7.0, 8.0, np.nan], {},),
(
"max",
Expand Down