ENH: provide standard BaseIndexers in pandas.api.indexers #33236

AlexKirko · 2020-04-02T12:36:58Z

closes ENH: provide standard BaseIndexers in pandas.api.indexers #33201
1 test modified / 1 passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

This PR adds a forward-looking pandas.api.indexers.BaseIndexer subclass implementation and exposes it to users as discussed in #33180 .

Exposing other indexers

It is also possible to expose a backward-looking indexer simply by exposing pandas.core.window.indexers.FixedWindowIndexer through the API. Seems redundant to me as this is basically the same as supplying an integer as the window argument when creating a rolling object but it can be done for consistency. I'd be grateful for a discussion.

Note on tests

The class is incorporated into test_rolling_forward_window in test_base_indexer.py and this serves as a test that the new indexer is working correctly.

Notes on performance

I've tested the implementation and it shows comparable performance to pandas.core.window.indexers.FixedWindowIndexer. In my tests the new indexer was slightly faster, either by luck, or because I streamlined the backward-looking indexer a bit when making the forward-looking one.

WillAyd · 2020-04-02T23:27:18Z

/azp run

azure-pipelines · 2020-04-02T23:27:27Z

Azure Pipelines successfully started running 1 pipeline(s).

jreback · 2020-04-03T20:01:32Z

looks good, if you can rebase and make the change to use the FixedForward indexer. Also I think worthile adding a benchmark.

jreback · 2020-04-03T20:01:51Z

@TomAugspurger @jorisvandenbossche @jbrockmendel if any comments.

AlexKirko · 2020-04-04T09:17:18Z

Travis failing has nothing to do with this PR. See #33280
Waiting for merge.

mroeschke · 2020-04-04T18:18:31Z

Could you document this new indexer around here: https://pandas.pydata.org/pandas-docs/stable/user_guide/computation.html#custom-window-rolling

AlexKirko · 2020-04-04T20:37:59Z

Found a bug in the benchmark, needs a bit more work.

UPDATE: fixed it and tested locally, the new benchmark is working now.

AlexKirko · 2020-04-04T21:33:50Z

@mroeschke Added documentation to the custom window rolling subsection. Can introduce a new subsection, but I think it fits in. Please tell me what you think.

AlexKirko · 2020-04-04T21:35:28Z

@jreback
Rebased, changed the test, added a benchmark to the suite. All green.

jbrockmendel · 2020-04-05T00:21:05Z

LGTM

jreback · 2020-04-05T19:29:42Z

doc/source/user_guide/computation.rst

+For some problems knowledge of the future is available for analysis. For example, this occurs when
+each data point is a full time series read from an experiment, and the task is to extract underlying
+conditions. In these cases it can be useful to perform forward-looking rolling window computations.
+``FixedForwardWindowIndexer`` class is available for this purpose. This ``BaseIndexer`` subclass


you can use the ref here, e.g. the one you are using in the whatsnew

@jreback I've included a ref in whatsnew, pointing to this subsection of the user guide. If you meant something different, please tell me.

no I mean a ref on FixedForwardIndexer here that points to the api (makes a clickable link), and one for BaseIndexer

jreback · 2020-04-05T19:30:00Z

doc/source/user_guide/computation.rst

+``FixedForwardWindowIndexer`` class is available for this purpose. This ``BaseIndexer`` subclass
+implements a closed fixed-width forward-looking rolling window, and we can use it as follows:
+
+.. code-block:: ipython


use an ipython block here

@jreback
Sorry, could you clarify? To me, my ipython block looks right, both in raw and in the docs I compiled locally. Do I need to do something differently?

we don't use code-blocks on executable code. pls use an ipython block so that the doc-build executes this, that way it will never get out of date.

pandas/core/window/indexers.py

jreback · 2020-04-05T19:31:51Z

pandas/core/window/indexers.py

+
+
+class FixedForwardWindowIndexer(BaseIndexer):
+    """Calculate fixed forward-looking rolling window bounds,"""


can you add Examples section here

jreback · 2020-04-05T19:32:35Z

pandas/core/window/indexers.py

+    def get_window_bounds(
+        self,
+        num_values: int = 0,
+        min_periods: Optional[int] = None,


this should raise if min_periods, center, closed are not None

Implemented it a bit differently, pinged you in the comment on the implementation.

AlexKirko · 2020-04-06T11:15:05Z

Looks like the doc build fail is being handled in #33309

AlexKirko · 2020-04-06T11:25:01Z

pandas/core/window/indexers.py

+        if closed is not None:
+            raise ValueError(
+                "Forward-looking windows don't support setting the closed argument"
+            )


@jreback
Implemented the error-raising behavior this way. Reasons below.

As far as I've been able to test, the arguments are being set by the rolling object in a general way. They are passed whether a particular BaseIndexer implementation needs them or not.
min_periods is getting set to rolling.min_periods. Same stuff for center: in my tests it got set to False by default. Only closed was set as None.
I think we should raise if something that doesn't make sense got passed, but testing for None is too crude for our purposes.

AlexKirko · 2020-04-06T15:39:30Z

@jreback
All green, pinged you on relevant changes. I think I've addressed everything, but I'm not sure if I understood you correctly on adding refs and the ipython block. Please tell me if I misunderstood anything.

jreback · 2020-04-06T21:48:51Z

doc/source/user_guide/computation.rst

+For some problems knowledge of the future is available for analysis. For example, this occurs when
+each data point is a full time series read from an experiment, and the task is to extract underlying
+conditions. In these cases it can be useful to perform forward-looking rolling window computations.
+``FixedForwardWindowIndexer`` class is available for this purpose. This ``BaseIndexer`` subclass


no I mean a ref on FixedForwardIndexer here that points to the api (makes a clickable link), and one for BaseIndexer

jreback · 2020-04-06T21:49:23Z

doc/source/user_guide/computation.rst

+``FixedForwardWindowIndexer`` class is available for this purpose. This ``BaseIndexer`` subclass
+implements a closed fixed-width forward-looking rolling window, and we can use it as follows:
+
+.. code-block:: ipython


we don't use code-blocks on executable code. pls use an ipython block so that the doc-build executes this, that way it will never get out of date.

jreback · 2020-04-06T21:50:18Z

pandas/core/window/indexers.py

+        if center:
+            raise ValueError("Forward-looking windows can't have center=True")
+        if closed is not None:
+            raise ValueError(


need a test that hits these

Done. Added to the test.

AlexKirko · 2020-04-07T11:48:00Z

@jreback
Thanks for the clarification! Sorry for getting confused: first time adding a public class.

Added an entry to the window.rst so that the docstrings get published as part of API reference output.
Made the links in the user guide clickable and tested locally.
Replaced the code-block with an ipython block. Thanks.
Added pytest.raises to test error-raising behavior. I think having it in the same test works, but please tell me if you'd rather have error-raising behavior be a separate test.

mroeschke · 2020-04-08T04:09:30Z

doc/source/reference/window.rst

@@ -85,3 +85,14 @@ Base class for defining custom window boundaries.
   :toctree: api/

   api.indexers.BaseIndexer
+
+Forward-looking fixed window indexer


I don't think you need to add a whole new section for this. Just place api.indexers.FixedForwardWindowIndexer right below api.indexers.BaseIndexer

Done.

Looks better now, thanks. Sorry, I'm still learning how our docs are built.

jreback · 2020-04-08T17:20:58Z

thanks @AlexKirko very nice!

AlexKirko mentioned this pull request Apr 2, 2020

CI/COMPAT: Linux py37_np_dev pipeline timeouts #33238

Closed

jreback added Enhancement Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Apr 3, 2020

jreback added this to the 1.1 milestone Apr 3, 2020

AlexKirko added 8 commits April 4, 2020 07:41

ENH: initial indexer implementation

31fd213

FIX: fix brackets

d2b7743

ENH: expose FixedForwardWIndowIndexer through API

5afb066

DOC: add whatsnew entry

212448f

CLN: run black

0aa7980

DOC: add example to Rolling docstring

fff110c

TST: use new class in test_rolling_forward_window

c94cbd5

TST: add benchmark

4e5aa02

AlexKirko force-pushed the forward-indexer branch from 6d4bb3f to 4e5aa02 Compare April 4, 2020 05:27

fix benchmark

271cc36

Merge branch 'master' into forward-indexer

ac8fd5f

AlexKirko added 2 commits April 4, 2020 21:46

DOC: document the indexer in the user guide

dc7fec8

DOC: edit the text in the user_guide

e1a6f73

fix the benchmark

dadcb3d

mroeschke approved these changes Apr 5, 2020

View reviewed changes

DOC: add versionadded to user_guide

46f45c2

jreback requested changes Apr 5, 2020

View reviewed changes

AlexKirko added 6 commits April 6, 2020 07:48

raise if min_periods, center, or closed is not None

7464c92

DOC: expand the class docstring

6cdd20d

DOC: add examples to the class docstring

fb43381

DOC: add ref from whatsnew to the user guide

9702839

rollback raising behavior

728ada2

add errors for center and closed

ffd50c5

AlexKirko added 2 commits April 6, 2020 14:17

CLN: run black

3ed0437

CLN: remove unnecessary quotes

1e3efdd

AlexKirko commented Apr 6, 2020

View reviewed changes

Merge branch 'master' into forward-indexer

916bd6a

AlexKirko requested a review from jreback April 6, 2020 15:39

jreback requested changes Apr 6, 2020

View reviewed changes

AlexKirko added 3 commits April 7, 2020 09:53

DOC: replace code-block with ipython block

9bc5fc3

DOC: expose docstring, make links clickable

bd265f4

TST: add error raising tests

d08081b

AlexKirko added 4 commits April 7, 2020 14:53

Merge branch 'master' into forward-indexer

6e7c629

CLN: clean up formatting in docs

81c4216

CLN: remove unnecessary BaseIndexer import

1451340

CLN: remove unnecessary new line in computation.rst

768197a

AlexKirko requested a review from jreback April 7, 2020 15:05

mroeschke reviewed Apr 8, 2020

View reviewed changes

DOC: move new indexer in window reference, delete extra

bc59bb7

jreback approved these changes Apr 8, 2020

View reviewed changes

jreback merged commit 717d805 into pandas-dev:master Apr 8, 2020

AlexKirko deleted the forward-indexer branch April 9, 2020 06:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: provide standard BaseIndexers in pandas.api.indexers #33236

ENH: provide standard BaseIndexers in pandas.api.indexers #33236

AlexKirko commented Apr 2, 2020 •

edited

Loading

WillAyd commented Apr 2, 2020

azure-pipelines bot commented Apr 2, 2020

jreback commented Apr 3, 2020

jreback commented Apr 3, 2020

AlexKirko commented Apr 4, 2020

mroeschke commented Apr 4, 2020

AlexKirko commented Apr 4, 2020 •

edited

Loading

AlexKirko commented Apr 4, 2020

AlexKirko commented Apr 4, 2020

jbrockmendel commented Apr 5, 2020

jreback Apr 5, 2020

AlexKirko Apr 6, 2020

jreback Apr 6, 2020

jreback Apr 5, 2020

AlexKirko Apr 6, 2020

jreback Apr 6, 2020

jreback Apr 5, 2020

AlexKirko Apr 6, 2020

jreback Apr 5, 2020

AlexKirko Apr 6, 2020

AlexKirko commented Apr 6, 2020

AlexKirko Apr 6, 2020 •

edited

Loading

AlexKirko commented Apr 6, 2020

jreback Apr 6, 2020

jreback Apr 6, 2020

jreback Apr 6, 2020

AlexKirko Apr 7, 2020

AlexKirko commented Apr 7, 2020 •

edited

Loading

mroeschke Apr 8, 2020

AlexKirko Apr 8, 2020 •

edited

Loading

jreback commented Apr 8, 2020



		class FixedForwardWindowIndexer(BaseIndexer):
		"""Calculate fixed forward-looking rolling window bounds,"""

ENH: provide standard BaseIndexers in pandas.api.indexers #33236

ENH: provide standard BaseIndexers in pandas.api.indexers #33236

Conversation

AlexKirko commented Apr 2, 2020 • edited Loading

Exposing other indexers

Note on tests

Notes on performance

WillAyd commented Apr 2, 2020

azure-pipelines bot commented Apr 2, 2020

jreback commented Apr 3, 2020

jreback commented Apr 3, 2020

AlexKirko commented Apr 4, 2020

mroeschke commented Apr 4, 2020

AlexKirko commented Apr 4, 2020 • edited Loading

AlexKirko commented Apr 4, 2020

AlexKirko commented Apr 4, 2020

jbrockmendel commented Apr 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexKirko commented Apr 6, 2020

AlexKirko Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

AlexKirko commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexKirko commented Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

AlexKirko Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

jreback commented Apr 8, 2020

AlexKirko commented Apr 2, 2020 •

edited

Loading

AlexKirko commented Apr 4, 2020 •

edited

Loading

AlexKirko Apr 6, 2020 •

edited

Loading

AlexKirko commented Apr 7, 2020 •

edited

Loading

AlexKirko Apr 8, 2020 •

edited

Loading