BUG: Fix exceptions when Series.interpolate's order parameter is missing or invalid

[nmusolino]

closes #24014
xref #10633

• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• 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

[codecov]

Codecov Report

Merging #25246 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #25246      +/-   ##
==========================================
- Coverage    91.9%    91.9%   -0.01%     
==========================================
  Files         166      166              
  Lines       52213    52210       -3     
==========================================
- Hits        47985    47981       -4     
- Misses       4228     4229       +1

Flag	Coverage Δ
#multiple	90.44% <100%> (-0.01%)	⬇️
#single	42.12% <0%> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/core/internals/blocks.py	94.02% <100%> (-0.09%)	⬇️
pandas/core/missing.py	92.56% <100%> (ø)	⬆️

---

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 2448e52...5bd3d4d. Read the comment docs.

[codecov]

Codecov Report

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

@@            Coverage Diff             @@
##           master   #25246      +/-   ##
==========================================
+ Coverage    91.9%   91.93%   +0.03%     
==========================================
  Files         166      166              
  Lines       52213    52182      -31     
==========================================
- Hits        47985    47974      -11     
+ Misses       4228     4208      -20

Flag	Coverage Δ
#multiple	90.47% <100%> (+0.03%)	⬆️
#single	42.14% <0%> (+0.01%)	⬆️

Impacted Files	Coverage Δ
pandas/core/internals/blocks.py	94.08% <100%> (-0.03%)	⬇️
pandas/core/missing.py	92.56% <100%> (ø)	⬆️
pandas/util/testing.py	87.83% <0%> (+0.09%)	⬆️
pandas/core/frame.py	96.81% <0%> (+0.16%)	⬆️
pandas/io/formats/console.py	86.27% <0%> (+19.6%)	⬆️

---

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 2448e52...69dd362. Read the comment docs.

[pep8speaks]

Hello @nmusolino! Thanks for updating the PR.

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

Comment last updated on February 11, 2019 at 12:36 Hours UTC

[gfyoung]

While we can, let's do same thing here: break up this test so that we can leverage pytest parameterization.

[nmusolino]

I agree with you that this test ought to be broken up and simplified.

I will try to do that tonight, but if it proves difficult, I may propose doing it in a separate PR.

[nmusolino]

Okay, ready for another look.

[gfyoung]

Let's use a fixture for this.

[nmusolino]

I understand where you’re coming from, with the same ”method” parameterization in the two tests. I guess the fixture would be called nontemporal_method_name in this approach?

Since this test uses two parameters (”method” and ”limit”), I think it’s a bit clearer to simply have two parallel pytest forms (in this case, two usages of pytest.mark.parmeterize), to make it clear that the Cartesian product of parameters is being used.

[gfyoung]

From a maintenance perspective, fixture is much preferred over multiple usages of the parameter.

I guess the fixture would be called nontemporal_method_name in this approach?

Yes, that would be reasonable.

[jreback]

lgtm. @gfyoung over to you.

[gfyoung]

Thanks @nmusolino !