-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
Implement Akima1DInterpolator #12833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This build failed https://travis-ci.org/pydata/pandas/jobs/121843938 likely because of the
Is there a minimum version of Thanks |
pandas/tests/test_generic.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls have an expected case and use assert_series_equal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added assert
|
you can you call this in a try: except to handle the scipy signature differences. (then if it doesn't work in the excpet, raise again). we don't have a min version. |
pandas/core/missing.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so need a test for this. on the 2.7_LOCALE build we install a really old scipy. We may need to install a somewhat newer one on another build to explicity test for this. When did this Akima1DInterpolate appear in scipy? and seems the axis parm was added in 0.17.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.14.0 (as per scipy/scipy@a0a3b38) I implemented a different check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok the 3.4 build has scipy==0.14.0 installed so pls verify that the test works corretctly (IOW it is NOT skipped, but passes).
remove scipy from ci/requirements-3.5_OSX. Make sure that you DO see some skips. I don't think we were testing having NO scipy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok , just remove scipy from ci/requirements-3.5_OSX and I think will be good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from ci/requirements-3.5_OSX.run ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep (we are including a version of scipy on EVERY build, so need to take it from one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
The two test failures seem to be unrelated. |
1 similar comment
|
The two test failures seem to be unrelated. |
pandas/core/missing.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you change this (and the pchip one to something like):
if method == 'akima':
alt_methods['akima'] = _akima_interpolate
Move to _akima_interpolate
from scipy.interpolate import Akima1DInterpolator # noqa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
|
pls add a whatsnew as well |
|
Added whatsnew |
pandas/core/missing.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this is fine
|
pls squash |
|
add a small note when to use this in : http://pandas-docs.github.io/pandas-docs-travis/missing_data.html#interpolation (you might want to break up that section a bit and make it into a table) |
|
add to the doc-string for |
|
Added notes and versionadded tag |
pandas/core/missing.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no this needs to be updated in pandas.core.generic.NDFrame.interpolate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved section.
|
Updated per comments. |
doc/source/missing_data.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good
|
ok, looks good. ping when green (and pls verify that tests are skipped as appropriate) |
doc/source/missing_data.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need two spaces here at the beginning of this line, so the start of the text is aligned with the start of the text in the previous line ('If ...')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pandas/core/generic.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'support for 'akima'' should go on the next line I think. See the sphinx docs: http://www.sphinx-doc.org/en/stable/markup/para.html#directive-versionadded for an example. And then maybe change the text to something like "Added support for the 'akima' method"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
|
@gliptak I just added some more doc comments, looks very good for the rest! |
|
@jreback Build is green. Tests skipped https://s3.amazonaws.com/archive.travis-ci.org/jobs/122101334/log.txt I also opened #12856 (consistently fails the same way) |
|
@gliptak thanks! pls review the built docs for accuracy (and good looking ness) and issue a follow up if necessary. (prob will take a few hours before they are built) |
|
@jreback @jorisvandenbossche Thank you for guiding this through |
|
Docs updated good. |
git diff upstream/master | flake8 --diff