BUG: datetime rolling min/max segfaults when closed=left (#21704) #21853

changhiskhan · 2018-07-11T03:16:01Z

User reported that df.rolling(to_offset('3D'), closed='left').max()
segfaults when df has a datetime index. The bug was in PR #19549. In
that PR, in https://github.com/pandas-dev/pandas/blame/master/pandas/_libs/window.pyx#L1268
i is initialized to endi[0], which is 0 when closed=left.
So in the next line when it tries to set output[i-1] it goes out of bounds.
In addition, there are 2 more bugs in the roll_min_max code.
The second bug is that for variable size windows, the nobs is never updated
when elements leave the window. The third bug is at the end of the fixed
window where all output elements up to minp are initialized to 0 if
the input is not float.

This PR fixes all three of the aforementioned bugs, at the cost of casting the
output array to floating point even if the input is integer. This is less
than ideal if the output has no NaNs, but is still consistent with roll_sum
behavior.

closes BUG: Offset-based rolling window, with closed='left' and max as aggregation function, make python crash #21704
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2018-07-11T03:16:05Z

Hello @changhiskhan! Thanks for updating the PR.

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

Comment last updated on July 13, 2018 at 05:02 Hours UTC

jbrockmendel · 2018-07-11T04:24:42Z

pandas/tests/test_window.py

@@ -3772,7 +3816,7 @@ def test_ragged_apply(self, raw):

        df = self.ragged

-        f = lambda x: 1
+        def f(x): return 1


The linter doesn’t like this. You need to either revert this change or put the return on the next line.

Looks like i accidentally had autopep8 or something on. i'll revert the change

jbrockmendel · 2018-07-11T04:25:01Z

pandas/tests/test_window.py

+                        index=pd.date_range('2000', periods=10))
+        ser[ser.index[-3:]] = np.nan
+        result = ser.rolling('3D', min_periods=2, closed='left').min()
+        print(ser)


Is this print needed?

no i'll take it out

jbrockmendel · 2018-07-11T04:26:24Z

pandas/_libs/window.pyx

+                    W.push_back(i)
+                output[0] = calc_mm(minp, nobs, input[Q.front()])
+                close_offset = 0
+            else: # if right is open then the first output is always NaN


Two spaces before comment

Thanks. Addressed in update.

codecov · 2018-07-11T15:58:33Z

Codecov Report

Merging #21853 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #21853   +/-   ##
=======================================
  Coverage   91.91%   91.91%           
=======================================
  Files         164      164           
  Lines       49992    49992           
=======================================
  Hits        45951    45951           
  Misses       4041     4041

Flag	Coverage Δ
#multiple	`90.3% <ø> (ø)`	⬆️
#single	`42.16% <ø> (ø)`	⬆️

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 365eac4...7b839e5. Read the comment docs.

changhiskhan · 2018-07-11T17:01:46Z

Not sure why the linter is failing on the travis-ci build. I updated my feature branch with E704 issues, and runnning ci/lint.sh locally passes just fine...

changhiskhan · 2018-07-11T17:02:53Z

ack, now it's reporting the travis-ci has passed. I guess it just didn't update yet after i pushed

jbrockmendel · 2018-07-11T17:33:04Z

@jreback this looks about ready, but I don't know this part of the code that well.

changhiskhan · 2018-07-11T17:59:54Z

Piggy-backing off this PR for a pandas-dev question.
When I run the unit tests locally, I get a few locale related failures that I don't see on CI. e.g., pandas/tests/series/test_datetime_values.py:305:
Shows:

../../anaconda3/lib/python3.6/locale.py:581: in getlocale
    return _parse_localename(localename)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

**localename = 'en_IL'**

I'm not even sure where the "en_IL" is coming from:

(base) chang@changhispad ~/code/pandas (gh21704)$ locale
LANG=en_US.UTF-8
LANGUAGE=
LC_CTYPE="en_US.UTF-8"
LC_NUMERIC="en_US.UTF-8"
LC_TIME="en_US.UTF-8"
LC_COLLATE="en_US.UTF-8"
LC_MONETARY="en_US.UTF-8"
LC_MESSAGES="en_US.UTF-8"
LC_PAPER="en_US.UTF-8"
LC_NAME="en_US.UTF-8"
LC_ADDRESS="en_US.UTF-8"
LC_TELEPHONE="en_US.UTF-8"
LC_MEASUREMENT="en_US.UTF-8"
LC_IDENTIFICATION="en_US.UTF-8"
LC_ALL=en_US.UTF-8

WillAyd · 2018-07-11T18:09:12Z

pandas/_libs/window.pyx

@@ -1241,49 +1242,69 @@ cdef _roll_min_max(ndarray[numeric] input, int64_t win, int64_t minp,
        input, win,
        minp, index, closed)

-    output = np.empty(N, dtype=input.dtype)
+    output = np.empty(N, dtype=float)


Is this coercing integer input to float?

yes, for two reasons:

because when closed=left the first output should be NaN.

because output elements up to minp should also be NaN.

I believe this behavior is consistent with rolling sum (coerced to float).

WillAyd · 2018-07-11T18:09:32Z

pandas/_libs/window.pyx

        ndarray[int64_t] starti, endi
-        ndarray[numeric, ndim=1] output
+        ndarray[double_t, ndim=1] output


What was the reason for changing the type here?

same as above

WillAyd · 2018-07-11T18:11:06Z

pandas/tests/test_window.py

@@ -464,6 +464,49 @@ def test_closed(self):
        with pytest.raises(ValueError):
            df.rolling(window=3, closed='neither')

+    def test_closed_min_max_datetime(self):


Can you parametrize this test?

addressed in update. Waiting for CI

jreback

lgtm. if you can parameterize that one test by the closed parameter & test a few differernt dtypes would be great. ping on green.

changhiskhan · 2018-07-12T02:19:13Z

actually, hold on, i think I found a problem with my fix. I'll ping again once it's ready. Thanks.

changhiskhan · 2018-07-13T02:29:35Z

sorry this was a little bit more involved than I thought. I ended up adding more test cases as well and separated the variable vs fixed window code into two different functions so it's a bit easier to read.

…21704) User reported that `df.rolling(to_offset('3D'), closed='left').max()` segfaults when df has a datetime index. The bug was in PR pandas-dev#19549. In that PR, in https://github.com/pandas-dev/pandas/blame/master/pandas/_libs/window.pyx#L1268 `i` is initialized to `endi[0]`, which is 0 when `closed=left`. So in the next line when it tries to set `output[i-1]` it goes out of bounds. In addition, there are 2 more bugs in the `roll_min_max` code. The second bug is that for variable size windows, the `nobs` is never updated when elements leave the window. The third bug is at the end of the fixed window where all output elements up to `minp` are initialized to 0 if the input is not float. This PR fixes these three bugs, at the cost of casting the output array to floating point even if the input is integer. This is less than ideal if the output has no NaNs, but is consistent with roll_sum behavior.

changhiskhan · 2018-07-13T15:51:48Z

@jreback I think this is ready for you to take another look now.

jreback · 2018-07-14T14:53:37Z

looks good @changhiskhan if you can just run the asvs for min/max window ops to make sure nothing has significatnly changed.

@WillAyd if any comments, merge on good asv results.

jreback · 2018-07-20T13:12:43Z

@WillAyd if you are ok, pls approve & merge

…21704) (pandas-dev#21853)

jbrockmendel reviewed Jul 11, 2018

View reviewed changes

changhiskhan force-pushed the gh21704 branch from ad22857 to 181d671 Compare July 11, 2018 15:58

WillAyd requested changes Jul 11, 2018

View reviewed changes

jreback requested changes Jul 12, 2018

View reviewed changes

jreback added Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode Numeric Operations Arithmetic, Comparison, and Logical operations labels Jul 12, 2018

changhiskhan force-pushed the gh21704 branch from 181d671 to 0ae0a80 Compare July 12, 2018 02:07

changhiskhan force-pushed the gh21704 branch from 0ae0a80 to 084448d Compare July 13, 2018 02:21

changhiskhan force-pushed the gh21704 branch from 084448d to 7b839e5 Compare July 13, 2018 05:02

jreback added this to the 0.24.0 milestone Jul 14, 2018

jreback approved these changes Jul 14, 2018

View reviewed changes

WillAyd approved these changes Jul 20, 2018

View reviewed changes

WillAyd merged commit 0a0b2b9 into pandas-dev:master Jul 20, 2018

WillAyd added the Window rolling, ewma, expanding label Sep 4, 2018

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

BUG: datetime rolling min/max segfaults when closed=left (pandas-dev#…

89a73fa

…21704) (pandas-dev#21853)

mroeschke mentioned this pull request Oct 2, 2018

Rolling max and min on datetime column incorrect when NaN in window #22931

Closed

timhunderwood mentioned this pull request Jan 8, 2019

pandas.rolling.max() shut down reopen #24218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: datetime rolling min/max segfaults when closed=left (#21704) #21853

BUG: datetime rolling min/max segfaults when closed=left (#21704) #21853

changhiskhan commented Jul 11, 2018 •

edited

Loading

pep8speaks commented Jul 11, 2018 •

edited

Loading

jbrockmendel Jul 11, 2018

changhiskhan Jul 11, 2018

jbrockmendel Jul 11, 2018

changhiskhan Jul 11, 2018

jbrockmendel Jul 11, 2018

changhiskhan Jul 11, 2018

codecov bot commented Jul 11, 2018 •

edited

Loading

changhiskhan commented Jul 11, 2018

changhiskhan commented Jul 11, 2018

jbrockmendel commented Jul 11, 2018

changhiskhan commented Jul 11, 2018

WillAyd Jul 11, 2018

changhiskhan Jul 11, 2018

WillAyd Jul 11, 2018

changhiskhan Jul 11, 2018

WillAyd Jul 11, 2018

changhiskhan Jul 12, 2018

jreback left a comment

changhiskhan commented Jul 12, 2018

changhiskhan commented Jul 13, 2018

changhiskhan commented Jul 13, 2018

jreback commented Jul 14, 2018

jreback commented Jul 20, 2018

BUG: datetime rolling min/max segfaults when closed=left (#21704) #21853

BUG: datetime rolling min/max segfaults when closed=left (#21704) #21853

Conversation

changhiskhan commented Jul 11, 2018 • edited Loading

pep8speaks commented Jul 11, 2018 • edited Loading

Comment last updated on July 13, 2018 at 05:02 Hours UTC

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 Jul 11, 2018 • edited Loading

Codecov Report

changhiskhan commented Jul 11, 2018

changhiskhan commented Jul 11, 2018

jbrockmendel commented Jul 11, 2018

changhiskhan commented Jul 11, 2018

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

jreback left a comment

Choose a reason for hiding this comment

changhiskhan commented Jul 12, 2018

changhiskhan commented Jul 13, 2018

changhiskhan commented Jul 13, 2018

jreback commented Jul 14, 2018

jreback commented Jul 20, 2018

changhiskhan commented Jul 11, 2018 •

edited

Loading

pep8speaks commented Jul 11, 2018 •

edited

Loading

codecov bot commented Jul 11, 2018 •

edited

Loading