Repository: pandas-dev/pandas
Pull request: BUG: Fix exceptions when Series.interpolate's `order` parameter is missing or invalid
closes #24014
xref #10633
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Before this change, when the `Series.interpolate()` method was called with invalid arguments,  exceptions with informative messages would be raised internally, but then caught and suppressed.  An exception with a potentially misleading message would be raised instead.

For example, when interpolate is called with `method='spline'`, an `order` argument must also be supplied.  This is checked internally, but when the `order` argument was missing, a confusing error would be raised:
```
>>> import pandas as pd
>>> s = pd.Series([0, 1, pd.np.nan, 3, 4])
>>> s.interpolate(method='spline')
Traceback (most recent call last):
...
ValueError: invalid method 'spline' to interpolate.
```

This problem was reported as issues #10633 and #24014.

After this change, a specific and correct exception (previously caught and discarded internally) is raised:
```
>>> import pandas as pd
>>> s = pd.Series([0, 1, pd.np.nan, 3, 4])
>>> s.interpolate(method='spline')
...
ValueError: You must specify the order of the spline or polynomial.
```

In addition, light validation is now performed on the `order` parameter before it is passed to a `scipy` class.  Previously, `pandas` would check if `order` was truthy in the Python sense.  If `order` was non-zero and invalid, a scipy error would propagate to the user:
```
>>> s.interpolate(method='spline', order=-1)
Traceback (most recent call last):
...
dfitpack.error: (1<=k && k<=5) failed for 3rd argument k: fpcurf0:k=-1
```

After this change, a more understandable exception is raised in this case:
```
>>> s.interpolate(method='spline', order=-1)
Traceback (most recent call last):
...
ValueError: order needs to be specified and greater than 0
```

Problem statement:
Unnecessary bare except at class Block, function interpolate hides actual error
#### Code Sample, a copy-pastable example if possible

```python
# Minimal example:
import pandas as pd
df = pd.Series([0,1,pd.np.nan,3,4])
df.interpolate(method='spline')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "D:\venvs\food_and_drinking\lib\site-packages\pandas\core\generic.py", line 6034, in interpolate
    **kwargs)
  File "D:\venvs\food_and_drinking\lib\site-packages\pandas\core\internals.py", line 3702, in interpolate
    return self.apply('interpolate', **kwargs)
  File "D:\venvs\food_and_drinking\lib\site-packages\pandas\core\internals.py", line 3581, in apply
    applied = getattr(b, f)(**kwargs)
  File "D:\venvs\food_and_drinking\lib\site-packages\pandas\core\internals.py", line 1168, in interpolate
    raise ValueError("invalid method '{0}' to interpolate.".format(method))
ValueError: invalid method 'spline' to interpolate.
```
##### Expected output
```python
ValueError: You must specify the order of the spline or polynomial.
```

#### Problem description

If interpolation parameter not specified, it raises an error, which states invalid method
internals.py:1152:1155
        try:
            m = missing.clean_interp_method(method, **kwargs)
        except:
            m = None

If there is no such try/except block around the missing.clean_interp_method function call, we would get the proper exception from mising.py/clean_interp_method.

Pandas version: 0.23.4

Hints from issue discussion:
Do you have a minimal example? http://matthewrocklin.com/blog/work/2018/02/28/minimal-bug-reports
Minimal example:
import pandas as pd
df = pd.Series([0,1,pd.np.nan,3,4])
df.interpolate(method='spline')
For clarity, the expected output is 

```
ValueError: You must specify the order of the spline or polynomial.
```

@seboktamas can you edit your original post to include the minimal example and the expected output?

Commit message: Actually validate `order` before use in spline

Patch to review:
diff --git a/pandas/core/internals/blocks.py b/pandas/core/internals/blocks.py
index ac7d21de442db..47ee6dacdd001 100644
--- a/pandas/core/internals/blocks.py
+++ b/pandas/core/internals/blocks.py
@@ -1115,11 +1115,8 @@ def check_int_bool(self, inplace):
                                                fill_value=fill_value,
                                                coerce=coerce,
                                                downcast=downcast)
-        # try an interp method
-        try:
-            m = missing.clean_interp_method(method, **kwargs)
-        except ValueError:
-            m = None
+        # validate the interp method
+        m = missing.clean_interp_method(method, **kwargs)
 
         if m is not None:
             r = check_int_bool(self, inplace)
diff --git a/pandas/core/missing.py b/pandas/core/missing.py
index cc7bdf95200d1..98aac3d094b93 100644
--- a/pandas/core/missing.py
+++ b/pandas/core/missing.py
@@ -293,8 +293,8 @@ def _interpolate_scipy_wrapper(x, y, new_x, method, fill_value=None,
                                     bounds_error=bounds_error)
         new_y = terp(new_x)
     elif method == 'spline':
-        # GH #10633
-        if not order:
+        # GH #10633, #24014
+        if order is None or not (0 < order):
             raise ValueError("order needs to be specified and greater than 0")
         terp = interpolate.UnivariateSpline(x, y, k=order, **kwargs)
         new_y = terp(new_x)
diff --git a/pandas/tests/series/test_missing.py b/pandas/tests/series/test_missing.py
index 985288c439917..ac2de38f95fa6 100644
--- a/pandas/tests/series/test_missing.py
+++ b/pandas/tests/series/test_missing.py
@@ -1064,19 +1064,32 @@ def test_interp_limit(self):
         # GH 9217, make sure limit is an int and greater than 0
         methods = ['linear', 'time', 'index', 'values', 'nearest', 'zero',
                    'slinear', 'quadratic', 'cubic', 'barycentric', 'krogh',
-                   'polynomial', 'spline', 'piecewise_polynomial', None,
+                   'polynomial', 'spline', 'piecewise_polynomial',
                    'from_derivatives', 'pchip', 'akima']
         s = pd.Series([1, 2, np.nan, np.nan, 5])
         msg = (r"Limit must be greater than 0|"
                "time-weighted interpolation only works on Series or"
                r" DataFrames with a DatetimeIndex|"
-               r"invalid method '(polynomial|spline|None)' to interpolate|"
-               "Limit must be an integer")
+               r"Limit must be an integer|"
+               r"You must specify the order of the spline")
         for limit in [-1, 0, 1., 2.]:
             for method in methods:
                 with pytest.raises(ValueError, match=msg):
                     s.interpolate(limit=limit, method=method)
 
+    def test_interp_invalid_method(self):
+        s = Series([1, 3, np.nan, 12, np.nan, 25])
+
+        invalid_methods = [None, 'nonexistent_method']
+        for method in invalid_methods:
+            msg = "method must be one of.*\\. Got '{}' instead".format(method)
+            with pytest.raises(ValueError, match=msg):
+                s.interpolate(method=method)
+            # When an invalid method and invalid limit (such as -1) are
+            # provided, the error message reflects the invalid method.
+            with pytest.raises(ValueError, match=msg):
+                s.interpolate(method=method, limit=-1)
+
     def test_interp_limit_forward(self):
         s = Series([1, 3, np.nan, np.nan, np.nan, 11])
 
@@ -1277,7 +1290,7 @@ def test_interp_limit_no_nans(self):
     @pytest.mark.parametrize("method", ['polynomial', 'spline'])
     def test_no_order(self, method):
         s = Series([0, 1, np.nan, 3])
-        msg = "invalid method '{}' to interpolate".format(method)
+        msg = "You must specify the order of the spline or polynomial"
         with pytest.raises(ValueError, match=msg):
             s.interpolate(method=method)
 
@@ -1315,16 +1328,17 @@ def test_spline_interpolation(self):
 
     @td.skip_if_no_scipy
     def test_spline_error(self):
-        # see gh-10633
+        # see GH-10633, GH-24014
         s = pd.Series(np.arange(10) ** 2)
         s[np.random.randint(0, 9, 3)] = np.nan
-        msg = "invalid method 'spline' to interpolate"
+        msg = "You must specify the order of the spline or polynomial"
         with pytest.raises(ValueError, match=msg):
             s.interpolate(method='spline')
 
         msg = "order needs to be specified and greater than 0"
-        with pytest.raises(ValueError, match=msg):
-            s.interpolate(method='spline', order=0)
+        for invalid_order in [-1, -1., 0, 0., np.nan]:
+            with pytest.raises(ValueError, match=msg):
+                s.interpolate(method='spline', order=invalid_order)
 
     def test_interp_timedelta64(self):
         # GH 6424


Code review comments:
diff_hunk: @@ -293,8 +293,8 @@ def _interpolate_scipy_wrapper(x, y, new_x, method, fill_value=None,
                                     bounds_error=bounds_error)
         new_y = terp(new_x)
     elif method == 'spline':
-        # GH #10633
-        if not order:
+        # GH #10633, #24014
+        if order is None or not (0 < order):
path: pandas/core/missing.py
text: @user1:
`not (0 < order)` --> `order >= 0` ?

@author:
Done.  I had written it the original way so that it would raise on `order=numpy.nan`.  After the simplification, `numpy.nan` won't trigger the exception, but I think that's a reasonable balance.

@user1:
Hmm...I see.  Then let's add a comment about that.

@author:
Do you think we should catch the case where `order` is nan?  I can add a comment either way, as you suggest.

@user1:
Actually, you can use `pandas.isnull`, which would simplify this i.e.

`if pandas.isnull(order) or order >= 0`

Then we wouldn't even need the comment.

@author:
Of course, that's a good suggestion.  I used `isna`, which is already imported in the module and would catch a case of `None`.  

@user1:
Yep, that works, since `isna` and `isnull` are one in the same function.