ENH: Raise useful error when iterating a Window #20996

selik · 2018-05-09T22:12:22Z

Until Issue #11704 is completed, raise a NotImplementedError to provide
a more clear error message when attempting to iterate over a Rolling
or Expanding window.

Until Issue pandas-dev#11704 is completed, raise a NotImplementedError to provide a more clear error message when attempting to iterate over a Rolling or Expanding window.

codecov · 2018-05-09T23:09:58Z

Codecov Report

Merging #20996 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #20996      +/-   ##
==========================================
+ Coverage   91.82%   91.82%   +<.01%     
==========================================
  Files         153      153              
  Lines       49488    49507      +19     
==========================================
+ Hits        45443    45462      +19     
  Misses       4045     4045

Flag	Coverage Δ
#multiple	`90.22% <100%> (ø)`	⬆️
#single	`41.88% <50%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/window.py	`96.28% <100%> (ø)`	⬆️
pandas/core/algorithms.py	`94.5% <0%> (ø)`	⬆️
pandas/core/strings.py	`98.62% <0%> (ø)`	⬆️
pandas/core/reshape/tile.py	`93.37% <0%> (ø)`	⬆️
pandas/core/frame.py	`97.22% <0%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`95.77% <0%> (+0.01%)`	⬆️
pandas/core/indexes/interval.py	`93.14% <0%> (+0.05%)`	⬆️
pandas/core/indexes/timedeltas.py	`91.24% <0%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eff1faf...6bd748d. Read the comment docs.

WillAyd

Thanks for the submission - can you add a test to go with this?

WillAyd · 2018-05-10T02:39:57Z

pandas/core/window.py

@@ -181,6 +181,10 @@ def __unicode__(self):
        return "{klass} [{attrs}]".format(klass=self._window_type,
                                          attrs=','.join(attrs))

+    def __iter__(self):
+        url = 'https://github.com/pandas-dev/pandas/issues/11704'
+        raise NotImplementedError('See issue #11704 %s' % (url,))


Can you use .format here instead? Alternately could get away with just referencing the issue number

TomAugspurger · 2018-05-10T14:29:56Z

We'll want a test and a note in the whatsnew.

selik · 2018-05-10T16:44:36Z

@TomAugspurger @WillAyd Would the test be to confirm that a NotImplementedError is raised or to fail when it's raised?

pep8speaks · 2018-05-10T17:27:33Z

Hello @selik! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on May 12, 2018 at 16:13 Hours UTC

selik · 2018-05-10T17:31:47Z

I added tests for both rolling and expanding in the new functional pytest style. I wasn't sure from the contribution guide whether to continue with the class-organized style of the existing tests or to use the new style. https://pandas.pydata.org/pandas-docs/stable/contributing.html#transitioning-to-pytest

One issue with the functional pytest style is that parameterizing on fixtures is awkward. pytest-dev/pytest#349

WillAyd · 2018-05-10T19:46:26Z

doc/source/whatsnew/v0.23.0.txt

@@ -966,6 +966,7 @@ Other API Changes
 - Constructing a Series from a list of length 1 no longer broadcasts this list when a longer index is specified (:issue:`19714`, :issue:`20391`).
 - :func:`DataFrame.to_dict` with ``orient='index'`` no longer casts int columns to float for a DataFrame with only int and float columns (:issue:`18580`)
 - A user-defined-function that is passed to :func:`Series.rolling().aggregate() <pandas.core.window.Rolling.aggregate>`, :func:`DataFrame.rolling().aggregate() <pandas.core.window.Rolling.aggregate>`, or its expanding cousins, will now *always* be passed a ``Series``, rather than a ``np.array``; ``.apply()`` only has the ``raw`` keyword, see :ref:`here <whatsnew_0230.enhancements.window_raw>`. This is consistent with the signatures of ``.aggregate()`` across pandas (:issue:`20584`)
+- Window types, such as Rolling and Expanding, raise `NotImplementedError` upon iteration. This will ideally be replaced by mimicking the (key, group) iteration of GroupBy (:issue:`11704`).


Use double backticks around literals like NotImplementedError. Could also simply by just saying "Rolling and Expanding types will now raise a ``NotImplementedError`` upon iteration (:issue:`11704`)"

You may be right on how that eventually gets implemented but I wouldn't mention it here because who knows what change it could be subject to

WillAyd · 2018-05-10T19:52:17Z

pandas/tests/test_window.py

@@ -46,6 +46,27 @@ def win_types_special(request):
    return request.param


+# Issue 11704: Iteration over a Window
+
+@pytest.fixture


The fixtures here are a little overkill. Just parametrize a "klass" of pd.Series and pd.DataFrame (both can be constructed from the values provided).

WillAyd · 2018-05-10T19:52:52Z

pandas/tests/test_window.py

+    return pd.DataFrame({'a': [1, 2, 3, 4], 'b': [10, 20, 30, 40]})
+
+@pytest.mark.parametrize('which', [series(), frame()])
+def test_rolling_iterator(which):


Move this to TestRolling and call it test_iterator_raises

WillAyd · 2018-05-10T19:53:02Z

pandas/tests/test_window.py

+        iter(which.rolling(2))
+
+@pytest.mark.parametrize('which', [series(), frame()])
+def test_expanding_iterator(which):


Move this to TestExpanding and call it test_iterator_raises

WillAyd

Minor edits (apply to both test cases) otherwise lgtm

WillAyd · 2018-05-12T16:00:35Z

pandas/tests/test_window.py

@@ -533,6 +512,14 @@ def test_multi_index_names(self):
        tm.assert_index_equal(result.columns, df.columns)
        assert result.index.names == [None, '1', '2']

+    @pytest.mark.parametrize('cls', [pd.Series, pd.DataFrame])


Can you use "klass" instead of "cls"

I strongly prefer cls in this case. Certain misspellings have a distasteful cultural undertone for me. Also, cls is an equally popular standard in other Python projects, if not more popular.

"klass" is most used in pandas so we should stick with that as the standard for now (thought admittedly not 100%). If you feel so inclined you can definitely bring up the conversation to standard as an issue or on the mailing list and get feedback.

Would be easier to change in one sweep that having each contributor implement their own standard

grep "parametrize.*klass" -r pandas/tests/ | wc -l 38 grep "parametrize.*box" -r pandas/tests/ | wc -l 12 grep "parametrize.*cls" -r pandas/tests/ | wc -l 10

Ok, I'll make that change separately.

WillAyd · 2018-05-12T16:00:44Z

pandas/tests/test_window.py

@@ -533,6 +512,14 @@ def test_multi_index_names(self):
        tm.assert_index_equal(result.columns, df.columns)
        assert result.index.names == [None, '1', '2']

+    @pytest.mark.parametrize('cls', [pd.Series, pd.DataFrame])
+    def test_iter_raises(cls):


Missing "self" as first argument

WillAyd

lgtm - thanks!

TomAugspurger · 2018-05-12T18:38:20Z

Thanks!

* ENH: Raise useful error when iterating a Window Until Issue pandas-dev#11704 is completed, raise a NotImplementedError to provide a more clear error message when attempting to iterate over a Rolling or Expanding window.

ENH: Raise useful error when iterating a Window

fe9e310

Until Issue pandas-dev#11704 is completed, raise a NotImplementedError to provide a more clear error message when attempting to iterate over a Rolling or Expanding window.

WillAyd requested changes May 10, 2018

View reviewed changes

jreback added the Error Reporting Incorrect or improved errors from pandas label May 10, 2018

selik added 2 commits May 10, 2018 10:26

Test that iteration raises NotImplemented

9fd89dc

Note window iteration raises NotImplemented

4ac67f0

switch % to .format

15224fe

WillAyd requested changes May 10, 2018

View reviewed changes

selik added 2 commits May 12, 2018 08:55

style change as per suggestion

4b44b05

fix testfunc parameters

1bcaa7f

WillAyd requested changes May 12, 2018

View reviewed changes

s/cls/klass/g

6bd748d

WillAyd approved these changes May 12, 2018

View reviewed changes

TomAugspurger merged commit 3283ae8 into pandas-dev:master May 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Raise useful error when iterating a Window #20996

ENH: Raise useful error when iterating a Window #20996

selik commented May 9, 2018

codecov bot commented May 9, 2018 •

edited

Loading

WillAyd left a comment

WillAyd May 10, 2018

TomAugspurger commented May 10, 2018

selik commented May 10, 2018

pep8speaks commented May 10, 2018 •

edited

Loading

selik commented May 10, 2018 •

edited

Loading

WillAyd May 10, 2018

WillAyd May 10, 2018

WillAyd May 10, 2018

WillAyd May 10, 2018

WillAyd left a comment

WillAyd May 12, 2018

selik May 12, 2018 •

edited

Loading

WillAyd May 12, 2018

selik May 12, 2018

WillAyd May 12, 2018

WillAyd left a comment

TomAugspurger commented May 12, 2018

ENH: Raise useful error when iterating a Window #20996

ENH: Raise useful error when iterating a Window #20996

Conversation

selik commented May 9, 2018

codecov bot commented May 9, 2018 • edited Loading

Codecov Report

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented May 10, 2018

selik commented May 10, 2018

pep8speaks commented May 10, 2018 • edited Loading

Comment last updated on May 12, 2018 at 16:13 Hours UTC

selik commented May 10, 2018 • edited Loading

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

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selik May 12, 2018 • edited Loading

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

WillAyd left a comment

Choose a reason for hiding this comment

TomAugspurger commented May 12, 2018

codecov bot commented May 9, 2018 •

edited

Loading

pep8speaks commented May 10, 2018 •

edited

Loading

selik commented May 10, 2018 •

edited

Loading

selik May 12, 2018 •

edited

Loading