The roll_quantile function now throws an exception instead of causing a segfault #15476
Conversation
… the quantile value is not in [0.0, 1.0]
|
@csizsek great! please add a whatsnew note in Bug Fixes (0.20.0). otherwise lgtm. (FYI, you can also tests this (already works, but just to have a test) |
jreback
added
Algos
…en called with a string.
|
@jreback Thanks for the quick review and the help, I added the test you suggested, but I'm not sure how to do this: "please add a whatsnew note in Bug Fixes (0.20.0)". What file do I need to modify? |
|
https://github.com/pandas-dev/pandas/blob/master/doc/source/whatsnew/v0.20.0.txt use a blank line in the Bug Fix section (not at the end) |
|
Thanks, done. Do I need to do anything else or can we consider this as fixed? |
|
next step is to ping on green. lgtm otherwise though. |
pandas/tests/test_window.py
Outdated
| @@ -1084,6 +1084,15 @@ def alt(x): | |||
|
|
|||
| self._check_moment_func(f, alt, name='quantile', quantile=q) | |||
|
|
|||
| self.assertRaises(ValueError, mom.rolling_quantile, | |||
| np.array([1, 2, 3]), window=3, quantile=-0.1) | |||
There was a problem hiding this comment.
actually make this a separate function, and don't use an array, instead use the new format
Series(....).rolling(3). We are dropping pd.rolling_quantile soon anyhow (as its been deprecated for a while).
There was a problem hiding this comment.
To avoid extra lambdas, can use:
with self.assertRaises(ValueError):
...There was a problem hiding this comment.
Thx for the feedback, fixed.
| @@ -1285,6 +1285,9 @@ def roll_quantile(ndarray[float64_t, cast=True] input, int64_t win, | |||
| ndarray[int64_t] start, end | |||
| ndarray[double_t] output | |||
|
|
|||
| if quantile < 0.0 or quantile > 1.0: | |||
There was a problem hiding this comment.
Can use a single 0 <= quantile <= 1 expression. Mind the endpoints: can't the chosen quantile be 0 or 1?
Oh, never mind. 😊
There was a problem hiding this comment.
Yes, it can, please see the PR
…lling.quantile().
pandas/tests/test_window.py
Outdated
| np.array([1, 2, 3]), window=3, quantile=10) | ||
| with self.assertRaises(ValueError): | ||
| ser.rolling(3).quantile(-0.1) | ||
| ser.rolling(3).quantile(10.0) |
There was a problem hiding this comment.
The second call needs it's own with block, otherwise the first call's risen ValueError might (in theory) mask the second one's, or vice versa.
Codecov Report
@@ Coverage Diff @@
## master #15476 +/- ##
==========================================
+ Coverage 90.35% 90.36% +<.01%
==========================================
Files 135 135
Lines 49513 49519 +6
==========================================
+ Hits 44739 44747 +8
+ Misses 4774 4772 -2
Continue to review full report at Codecov.
|
|
thanks @csizsek ! keep em coming! |
|
Sure. Thanks for the help! |
…using a segfault when quantile is out of range closes pandas-dev#15463 Author: Peter Csizsek <[email protected]> Closes pandas-dev#15476 from csizsek/fix-rolling-quantile-segfault and squashes the following commits: e31e5be [Peter Csizsek] Correctly catching exception in the test for Rolling.quantile. 4eea34a [Peter Csizsek] Refactored and moved exception throwing test to a new function for Rolling.quantile(). 8b1e020 [Peter Csizsek] Added a note about the Rolling.quantile bug fix to the changelog. f39b122 [Peter Csizsek] Added a new test case to roll_quantile_test to trigger a TypeError when called with a string. f736ca2 [Peter Csizsek] The roll_quantile function in window.pyx now raises a ValueError when the quantile value is not in [0.0, 1.0]
The roll_quantile function in window.pyx now raises a ValueError when the quantile value is not in [0.0, 1.0]. Also added tests for the new exception and extended test cases for the roll_quantile function.
git diff upstream/master | flake8 --diffbecause of some cdef code blocks in the file but my code does not generate any errors