Initial implementation of rolling iterators #27399
Conversation
|
Hello @ThomasKluiters! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-07-21 11:48:05 UTC |
|
Can you add tests for this? |
|
Also I think yes to both of your questions |
I wouldn't mind adding tests! I had to know what the expected output would be, but now that I know I'll be writing them! Thanks :) |
|
@mroeschke I have changed the files according to your feedback, could you have a look? |
They're already there, view the changes under test_expanding :) |
|
@ThomasKluiters thanks - sorry misread that on my end |
@mroeschke |
|
For the |
Well, it turns out that this required a bit of work - I've added some tests for it and had to change some code in window.py |
pandas/core/window.py
Outdated
| if arr.ndim == 1: | ||
| arr = np.expand_dims(arr, axis=1) | ||
|
|
||
| counts = libwindow.roll_sum( |
There was a problem hiding this comment.
Instead of this, could you just generate the slice and yield if the number non-null elements meets _minp?
result = values.iloc[slice(s, e)]
if result.count() > _minp:
yield result
There was a problem hiding this comment.
can you add a small section in computation.rst which shows off the iterator, probably in the rolling parts would work well.
I had a thought to co-locate all of these tests in pandas/tests/window/test_iterator.py (rather than splitting them off to the sub-files). I am not convinced our current organziation of tests here is that great.
otherwise lgtm.
doc/source/whatsnew/v0.25.0.rst
Outdated
| @@ -1206,6 +1206,7 @@ Groupby/resample/rolling | |||
| - Improved :class:`pandas.core.window.Rolling`, :class:`pandas.core.window.Window` and :class:`pandas.core.window.EWM` functions to exclude nuisance columns from results instead of raising errors and raise a ``DataError`` only if all columns are nuisance (:issue:`12537`) | |||
| - Bug in :meth:`pandas.core.window.Rolling.max` and :meth:`pandas.core.window.Rolling.min` where incorrect results are returned with an empty variable window (:issue:`26005`) | |||
| - Raise a helpful exception when an unsupported weighted window function is used as an argument of :meth:`pandas.core.window.Window.aggregate` (:issue:`26597`) | |||
| - Improved :class:`pandas.core.window.Rolling` and :class:`pandas.core.window.Window` it is now possible to iterate over a window object (:issue:`12537`) | |||
There was a problem hiding this comment.
also provide a reference to the new doc section.
| ], | ||
| ) | ||
| def test_iterator_dataframe(self, dataframe, expected, window): | ||
| expected = [DataFrame(values, index=index) for (values, index) in expected] |
There was a problem hiding this comment.
can you add the issue number as a comment here
| ], | ||
| ) | ||
| def test_iterator_series(self, series, expected, window): | ||
| expected = [Series(values, index=index) for (values, index) in expected] |
|
I have moved the tests to Though, I can't seem to figure out how to reference computation.rst in the Whatsnew, could you help me with this please :)? Thanks! |
|
can you merge master and see if can get passing |
|
IIRC this looked reasonable, can you merge master and update to any comments |
|
closing this as stale, though its a nice change, needs a rebase and comments addressed. |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffHi, I have yet to implements tests, use flake8 and create a whatsnew entry as I am uncertain whether this implementation would be correct.
I would really appreciate it if someone could give me some feedback on the code.
This PR will implement iterators for window functions.
Some questions I have: