Allow rolling API to accept BaseIndexer subclass#2
Conversation
|
closing and opening to trigger travis |
|
ok looks like travis is connected |
|
I also added some branches to prevent force pushing / deletion but no other restrictions: https://github.com/twosigma/pandas/settings/branches can edit / change if needed. |
|
Thanks for setting up Travis, Jeff. |
jreback
left a comment
jreback
left a comment
There was a problem hiding this comment.
lgtm so far. As mentioned, doing a couple of pre-cursor PRs to master makes sense to add typing & move tests to the appropriate place make sense.
|
|
||
| class BaseIndexer(abc.ABC): | ||
|
|
||
| def __init__(self, index, offset, keys): |
There was a problem hiding this comment.
whatever you can type would be great
|
|
||
| Parameters | ||
| ---------- | ||
| kwargs |
There was a problem hiding this comment.
can we explicity set the kwargs?
There was a problem hiding this comment.
I think we want to allow indexer authors to pass arbitrary data here.
There was a problem hiding this comment.
Scratch that, these should be explicit.
| import numpy as np | ||
|
|
||
| import pandas._libs.window as libwindow | ||
| import pandas._libs._window as libwindow_refactor |
There was a problem hiding this comment.
so I would either do this directly in .window or name this something non-private (as its already private). Note that if say refactoring needs to be done to make things cleaner we can do pre-cursors to master.
| Parameters | ||
| ---------- | ||
| window : int, or offset | ||
| window : int, offset, or BaseIndexer subclass |
There was a problem hiding this comment.
ideally type as much as possible (again I know that things are not typed now), maybe makes sense to do a pre-cursor to type what we have now
| return request.param | ||
|
|
||
|
|
||
| @pytest.fixture(params=['right', 'left', 'both', 'neither']) |
There was a problem hiding this comment.
some of this seems a good candidate for a pre-cursor PR to master (the fixtures)
| expected2 = ss.rolling(3, min_periods=1).cov() | ||
| tm.assert_series_equal(result, expected2) | ||
|
|
||
|
|
There was a problem hiding this comment.
not averse to actually putting the tests in a separate file (may make review easier).
so I think a pre-cursor PR to move pandas/tests/test_window.py -> pandas/tests/window/test_window.py makes sense (then we can add on things in an easy namespace)
|
|
||
| def _offset(window, center): | ||
| # TODO: (MATT) If the window is a BaseIndexer subclass, | ||
| # we need to pass in the materialized window |
There was a problem hiding this comment.
What is the type of window here? It looks like it can be a sequence or an integer?
There was a problem hiding this comment.
Correct. After a light audit, I anticipate the materialized window may be passed here and other times and Indexer class might be passed here.
Overall I think this routine is for label formatting.
| tm.assert_series_equal(result, expected2) | ||
|
|
||
|
|
||
| class TestCustomIndexer: |
There was a problem hiding this comment.
This class isn't necessary. Since we have pytest there's really no reason not to use top-level-function-style tests.
| window | ||
|
|
||
| """ | ||
| return NotImplemented |
mroeschke
changed the title
3 participants
This PR just allows the current rolling API to accept BaseIndexer subclasses in all the validation checks.
Some discussion points about
BaseIndexerI was thinking that
get_window_boundsshould accept additional keyword arguments from the rolling API likewin_type,closed,center, andwin_typejust in case the author wants to utilize those arguments in the bounds calculation.Since we are instantiating the
BaseIndexerbefore passing it intorolling, I assumedBaseIndexerwould be accepting theindex,offset, andkeysin the init.Any feedback welcome!