BUG: invalid rolling window on empty input #21291

uds5501 · 2018-06-02T05:05:04Z

Added a raise Value Error for the case when window==0, Kindly refer to issue #21286 for the same

closes rolling on 0 window causes core dump #21286
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
A simple except cases added

Added a raise Value Error for case when window==0, Kindly refer to issue pandas-dev#21286 for the same

pep8speaks · 2018-06-02T05:05:06Z

Hello @uds5501! Thanks for updating the PR.

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

Comment last updated on June 08, 2018 at 14:07 Hours UTC

uds5501 · 2018-06-02T05:07:01Z

Respected Seniors, this is literally my first PR. Kindly do guide me if I do anything wrong here

Included white spaces along the operator at line 94

WillAyd · 2018-06-02T23:09:05Z

Thanks for the PR! This appears to have broken a few tests. Be sure to check out the contributing to pandas guide and follow the steps locally to make sure you don't get any test errors before pushing to GitHub:

https://pandas.pydata.org/pandas-docs/stable/contributing.html

jorisvandenbossche · 2018-06-04T12:31:23Z

Some of those tests were added explicitly to test this in #13441 (it was a PR to disallow negative windows, but explicitly tested windows of zero not too raise upon construction).
I am not fully sure why those were added, so I think you can actually change those tests.

jorisvandenbossche · 2018-06-04T12:32:21Z

@uds5501 also general feedback: can you give the PR a more informative title? (you can still edit the title, see 'edit' button at the right of it)

uds5501 · 2018-06-04T16:22:54Z

@jorisvandenbossche I would do that soon and thanks for the link

Added one more check for the windows == 0 problem

WillAyd · 2018-06-07T17:32:20Z

Can you add a test to ensure that this raises? Also will eventually want a whatsnew note (probably 0.24 at this point)

mroeschke · 2018-06-07T17:48:42Z

pandas/core/window.py

@@ -602,6 +604,8 @@ def validate(self):
        if isinstance(window, (list, tuple, np.ndarray)):
            pass
        elif is_integer(window):
+            if window == 0:


Instead, you can combine this check with the one below (i.e window <= 0) and then raise an appropriate error message

added windows (0) test cases

@mroeschke

worked on the suggestion by @mroeschke and combined two cases in one

WillAyd

Looks like there are some test failures that need to be addressed, but otherwise minor comments on current code

WillAyd · 2018-06-08T05:42:28Z

pandas/core/window.py

@@ -96,6 +96,8 @@ def is_freq_type(self):
        return self.win_type == 'freq'

    def validate(self):
+        if self.window == 0:


As @mroeschke pointed out can't you combine this with the check below? Don't think we need this

WillAyd · 2018-06-08T05:43:16Z

pandas/tests/test_window.py

@@ -402,16 +402,17 @@ def test_constructor(self, which):
            with pytest.raises(ValueError):
                c(window=2, min_periods=1, center=w)

+    # these tests seems unnecessary here 21291


Don't need this comment

WillAyd · 2018-06-08T05:44:14Z

pandas/tests/test_window.py

        with pytest.raises(ValueError):
            c(-1, win_type='boxcar')
+


Errant extra line here will fail LINTing

uds5501 · 2018-06-08T05:49:16Z

pandas/core/window.py

-            if window < 0:
-                raise ValueError("window must be non-negative")
+            if window <= 0:
+                raise ValueError("window must be positive")


@WillAyd Combined them as pointed out by @mroeschke

My mistake in looking at the git diff I didn't realize they were two different methods. That said, are both required? Doesn't the call to super here raise already in the case where window == 0?

@WillAyd To be really honest, I don't think that it's required either. Let me remove this

codecov · 2018-06-08T11:20:31Z

Codecov Report

Merging #21291 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #21291      +/-   ##
==========================================
+ Coverage   91.85%   91.89%   +0.03%     
==========================================
  Files         153      153              
  Lines       49546    49579      +33     
==========================================
+ Hits        45509    45559      +50     
+ Misses       4037     4020      -17

Flag	Coverage Δ
#multiple	`90.29% <100%> (+0.04%)`	⬆️
#single	`41.85% <0%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/window.py	`96.28% <100%> (ø)`	⬆️
pandas/core/reshape/melt.py	`97.34% <0%> (ø)`	⬆️
pandas/io/formats/csvs.py	`98.14% <0%> (+0.01%)`	⬆️
pandas/core/indexes/interval.py	`93.16% <0%> (+0.02%)`	⬆️
pandas/core/indexes/datetimes.py	`95.8% <0%> (+0.02%)`	⬆️
pandas/core/series.py	`94.17% <0%> (+0.05%)`	⬆️
pandas/core/reshape/pivot.py	`97.03% <0%> (+0.05%)`	⬆️
pandas/io/formats/style.py	`96.12% <0%> (+0.08%)`	⬆️
pandas/core/dtypes/missing.py	`92.52% <0%> (+0.57%)`	⬆️
pandas/plotting/_core.py	`83.53% <0%> (+1.14%)`	⬆️

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 4274b84...9f5745c. Read the comment docs.

uds5501 · 2018-06-08T11:33:33Z

@WillAyd Please check the corrections now and see if it is good enough

jreback

can you add a whatsnew note (will be for 0.23.2), so wait for that (soon)

jreback · 2018-06-08T11:36:30Z

pandas/core/window.py

-            if window < 0:
-                raise ValueError("window must be non-negative")
+            if window <= 0:
+                raise ValueError("please enter a positive window")


maybe : window must be > 0

@jreback is this a suggestion for ValueError message?

Updated the commit according to the suggestion

jreback

actually if u can put a whatsnew note for 0.23.1
can get this in (bug fixes in the rolling section)

jreback

can you add a whatsnew note (will be for 0.23.2), so wait for that (soon)

jreback · 2018-06-08T12:50:44Z

doc/source/whatsnew/v0.23.1.txt

@@ -52,6 +52,7 @@ Groupby/Resample/Rolling
 - Bug in :func:`DataFrame.agg` where applying multiple aggregation functions to a :class:`DataFrame` with duplicated column names would cause a stack overflow (:issue:`21063`)
 - Bug in :func:`pandas.core.groupby.GroupBy.ffill` and :func:`pandas.core.groupby.GroupBy.bfill` where the fill within a grouping would not always be applied as intended due to the implementations' use of a non-stable sort (:issue:`21207`)
 - Bug in :func:`pandas.core.groupby.GroupBy.rank` where results did not scale to 100% when specifying ``method='dense'`` and ``pct=True``
+- Bug in :func:`pandas.core.windows.Windows.validate` where windows parameter was accpeting a numeric value of 0 (:issue:`21286`) 


pls make this user facing, something like

Bug in :func:`pandas.DataFrame.rolling` and :func:`pandas.Series.rolling` which incorrectly accepted a 0 window size rather than raising

jreback · 2018-06-08T12:51:13Z

@WillAyd were your comments addressed?

jreback · 2018-06-08T14:46:25Z

lgtm. @WillAyd

WillAyd

lgtm as well. @jreback let me know if / when you want me to merge

jreback · 2018-06-08T16:26:02Z

thanks @uds5501 (and @WillAyd )

(cherry picked from commit 93be27d)

Referring to Issue pandas-dev#21286

efd03f9

Added a raise Value Error for case when window==0, Kindly refer to issue pandas-dev#21286 for the same

white spaces fixed

6a5a26c

Included white spaces along the operator at line 94

jreback changed the title ~~Referring to Issue #21286~~ BUG: invalid rolling window on empty input Jun 5, 2018

jreback added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Jun 5, 2018

Update window.py

70e76df

Added one more check for the windows == 0 problem

mroeschke reviewed Jun 7, 2018

View reviewed changes

uds5501 added 3 commits June 7, 2018 23:30

Update test_window.py

2cd9b5c

added windows (0) test cases

Update window.py

87b2417

worked on the suggestion by @mroeschke and combined two cases in one

Update test_window.py

0349bf0

WillAyd requested changes Jun 8, 2018

View reviewed changes

uds5501 commented Jun 8, 2018

View reviewed changes

uds5501 added 4 commits June 8, 2018 11:20

Update test_window.py

5aacfbb

Update window.py

4131f30

Update window.py

956a7f2

Update window.py

a69e0e0

jreback requested changes Jun 8, 2018

View reviewed changes

jreback added this to the 0.23.2 milestone Jun 8, 2018

Update window.py

c9c769e

Updated the commit according to the suggestion

jreback requested changes Jun 8, 2018

View reviewed changes

Update v0.23.1.txt

da21015

jreback requested changes Jun 8, 2018

View reviewed changes

jreback modified the milestones: 0.23.2, 0.23.1 Jun 8, 2018

uds5501 added 2 commits June 8, 2018 19:33

Update v0.23.1.txt

5ecc436

Update v0.23.1.txt

9f5745c

jreback approved these changes Jun 8, 2018

View reviewed changes

WillAyd approved these changes Jun 8, 2018

View reviewed changes

jreback merged commit 93be27d into pandas-dev:master Jun 8, 2018

TomAugspurger added the Needs Backport label Jun 12, 2018

TomAugspurger pushed a commit to TomAugspurger/pandas that referenced this pull request Jun 12, 2018

BUG: invalid rolling window on empty input (pandas-dev#21291)

85c0177

(cherry picked from commit 93be27d)

TomAugspurger pushed a commit that referenced this pull request Jun 12, 2018

BUG: invalid rolling window on empty input (#21291)

25a18b7

(cherry picked from commit 93be27d)

TomAugspurger removed the Needs Backport label Jun 12, 2018

david-liu-brattle-1 pushed a commit to david-liu-brattle-1/pandas that referenced this pull request Jun 18, 2018

BUG: invalid rolling window on empty input (pandas-dev#21291)

808bac7

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

BUG: invalid rolling window on empty input (pandas-dev#21291)

d8a1f96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: invalid rolling window on empty input #21291

BUG: invalid rolling window on empty input #21291

uds5501 commented Jun 2, 2018 •

edited

Loading

pep8speaks commented Jun 2, 2018 •

edited

Loading

uds5501 commented Jun 2, 2018

WillAyd commented Jun 2, 2018

jorisvandenbossche commented Jun 4, 2018

jorisvandenbossche commented Jun 4, 2018

uds5501 commented Jun 4, 2018

WillAyd commented Jun 7, 2018

mroeschke Jun 7, 2018

WillAyd left a comment

WillAyd Jun 8, 2018

WillAyd Jun 8, 2018

WillAyd Jun 8, 2018

uds5501 Jun 8, 2018

WillAyd Jun 8, 2018

uds5501 Jun 8, 2018

codecov bot commented Jun 8, 2018 •

edited

Loading

uds5501 commented Jun 8, 2018

jreback left a comment

jreback Jun 8, 2018

uds5501 Jun 8, 2018

jreback left a comment

jreback left a comment

jreback Jun 8, 2018

jreback commented Jun 8, 2018

jreback commented Jun 8, 2018

WillAyd left a comment

jreback commented Jun 8, 2018

BUG: invalid rolling window on empty input #21291

BUG: invalid rolling window on empty input #21291

Conversation

uds5501 commented Jun 2, 2018 • edited Loading

pep8speaks commented Jun 2, 2018 • edited Loading

Comment last updated on June 08, 2018 at 14:07 Hours UTC

uds5501 commented Jun 2, 2018

WillAyd commented Jun 2, 2018

jorisvandenbossche commented Jun 4, 2018

jorisvandenbossche commented Jun 4, 2018

uds5501 commented Jun 4, 2018

WillAyd commented Jun 7, 2018

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

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

codecov bot commented Jun 8, 2018 • edited Loading

Codecov Report

uds5501 commented Jun 8, 2018

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 8, 2018

jreback commented Jun 8, 2018

WillAyd left a comment

Choose a reason for hiding this comment

jreback commented Jun 8, 2018

uds5501 commented Jun 2, 2018 •

edited

Loading

pep8speaks commented Jun 2, 2018 •

edited

Loading

codecov bot commented Jun 8, 2018 •

edited

Loading