BUG: COMPAT:0.18 added scipy version check GH12887 #13007
Conversation
| 'piecewise_polynomial': interpolate.piecewise_polynomial_interpolate, | ||
| } | ||
| import scipy | ||
| if scipy.__version__ <= '0.17': |
There was a problem hiding this comment.
Could you move this up to be with the from scipy import interpolate and assign a variable like scipy_version = scipy.__version__. Then your if block would me if LooseVersion(scipy_version) <= LooseVersion('0.17'):, where LooseVersion comese from distutils.version.
There was a problem hiding this comment.
Also, maybe start with
alt_methods = {
'barycentric': interpolate.barycentric_interpolate,
'krogh': interpolate.krogh_interpolate,
}and then do your if block checking the version. That way you aren't repeating the other two methods.
There was a problem hiding this comment.
Actually, why don't use just start with
alt_methods = {
'barycentric': interpolate.barycentric_interpolate,
'krogh': interpolate.krogh_interpolate,
'from_derivatives': interpolate.BPoly.from_derivatives`,
}and then only add piecewise_polynomial if the scipy version is less than 0.18. That way people can write code that is compatible with both versions of scipy.
|
Could you also add a test in
|
TomAugspurger
added
the
Missing-data
TomAugspurger
added
the
Compat
| @@ -3455,7 +3455,7 @@ def replace(self, to_replace=None, value=None, inplace=False, limit=None, | |||
| ---------- | |||
| method : {'linear', 'time', 'index', 'values', 'nearest', 'zero', | |||
| 'slinear', 'quadratic', 'cubic', 'barycentric', 'krogh', | |||
| 'polynomial', 'spline' 'piecewise_polynomial', 'pchip', | |||
| 'polynomial', 'spline' 'from_derivatives', 'pchip', | |||
There was a problem hiding this comment.
we need to leave the original method piecewise_polynomial and add the new one from_derivatives. Then switch using the version of scipy. E.g. if > 0.17 and piecewise_polynomila -> from_derivatives. If passed from_derivatives I would just use it.
There was a problem hiding this comment.
So we need a check near the top of interpolate in pandas.core.generic as well right?
There was a problem hiding this comment.
no all of this validation stuff is in core/missing.py. all we need here is a doc note. e.g. versionadded 0.18.1 -> from_derivates (and how scipy < 0.18.0) works with this.
|
we need a test to assert all of this stuff. put in |
|
I'll incorporate all of this. Thanks for the feedback thus far! |
|
I'll write the tests once I get the 'okay' on |
|
always write tests first! that way can validate you are testing the right thing |
| if method == 'piecewise_polynomial' and LooseVersion(scipy_version) > \ | ||
| LooseVersion('0.17'): | ||
| raise ValueError("Method '{0}' is deprecated for Scipy > 0.17. " | ||
| "Use 'from_derivatives' instead".format(method)) |
There was a problem hiding this comment.
I raise a ValueError here, however it is isn't propagated back up to the calling function interpolate from pandas/core/internals.py, so open to suggestions assuming this approach is correct to handling it.
|
You're right, should have started there. I'll get to work on those. |
|
|
||
| .. versionadded:: 0.18.1 | ||
| Added support for the 'akima' method | ||
| Added interpolate method 'from_derivatives' which replaces | ||
| 'piecewise_polynomial' in scipy 0.18; backwards-compatible with | ||
| scipy < 0.18 |
|
@jmwoloso can you update |
|
Update to the most recent pandas dev you mean? |
|
yep |
|
Sure thing. Away from my comp currently but will do when I get back. |
| @@ -516,3 +516,4 @@ Bug Fixes | |||
| - Bug in ``.describe()`` resets categorical columns information (:issue:`11558`) | |||
| - Bug where ``loffset`` argument was not applied when calling ``resample().count()`` on a timeseries (:issue:`12725`) | |||
| - ``pd.read_excel()`` now accepts column names associated with keyword argument ``names``(:issue:`12870`) | |||
| - Compat with SciPy > 0.17 for deprecated 'piecewise_polynomial' interpolation method (:issue:`12887`) | |||
There was a problem hiding this comment.
put this in Other API changes section. Add a line in Enhancements for the addition of from_derivatives method as well (you can use the PR number there).
|
Here's a patch which makes this clean. Just needs tests. |
added scipy version check to _interpolate_scipy_wrapper to handle deprecation of 'piecewise_polynomial' method for scipy > 0.17 CLN: added 'from_derivatives' to 'replace' docs in pandas.core.generic GH12887 added 'from_derivatives' to 'replace._shared_docs' and noted deprecation of 'piecewise_polynomial' method DOC: referenced compatibility fix in 'whatsnew' docs
|
xref #13055 moved interpolation tests to a better location. |
added scipy version check to _interpolate_scipy_wrapper to handle deprecation of 'piecewise_polynomial' method for scipy > 0.17 CLN: added 'from_derivatives' to 'replace' docs in pandas.core.generic GH12887 added 'from_derivatives' to 'replace._shared_docs' and noted deprecation of 'piecewise_polynomial' method DOC: referenced compatibility fix in 'whatsnew' docs
|
ok here's a working / tested branch: https://github.com/jreback/pandas/tree/jmwoloso-compat_12887 though the tests work, they don't really exercise this too much (as they same results as using akima). this this is a very simple interp problem though. |
|
Quick question. Should I be using the latest dev branch of scipy and numpy while I'm doing this or perhaps the latest stable releases of each? I'm currently using I ask because after applying your patch, I get an to This change takes care of the AttributeError, which, I'm guessing was occurring because However, it then raises an error mentioning the expected shape of y: |
|
make sure you have the latest pull of my branch it works on all versions of scipy and numpy (that code is a previous version) |
|
Ok, will do. I was going crazy for minute... |
…ndas into compat_12887 Conflicts: pandas/core/missing.py
|
Pulled down your branch and merged it with mine, though I'm still getting the same errors when using I must be doing something wrong, but git says I'm up to date: |
|
push up your changes and I'll look |
|
Just pushed them. I did have a merge conflict that I had to fix, I was using |
|
yeah it looks like you have some older code here. |
|
I am going to merge the branch I had earlier. Would appreciate if you can push some better tests though. |
|
Yeah, I'll try rebasing with master again as well, I don't have |
|
@jmwoloso sorry for the rush on this. trying to get the release out the door tomorrow :). If you can push some tests that would be great (no big hurry on those though). |
|
Understand completely. I appreciate the help. I'll get those tests pushed once I see what you've written already for them. |
|
great! I just copied the akima exactly. It gives the same results, so might it might be right. But I'ld ideally like something better. (but don't go crazy). |
|
Haha, sounds good. I was eyeballing akima as well. I'll see what else I can come up with. Thanks again! This was fun (my git-fu needs lots of work still). I'll browse for some other issues to tackle once I get these tests done. |
git diff upstream/master | flake8 --diff@jreback @TomAugspurger