Skip to content
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

stub out inherited methods of Akima1DInterpolator #3744

Merged
merged 2 commits into from
Jul 13, 2014

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented Jun 18, 2014

As it stands, Akima1DInterpolator.from_spline does not create an Akima interpolant --- it just creates a pp representation of its spline argument instead.
Both from_spline and from_bernstein_basis are artifacts of inheritance from PPoly, hence stub them out with NotImplementedErrors.
(cross-ref gh-3742)

Also cross-reference Akima1D and Pchip interpolators in the docs, while I'm at it.

from_spline and from_bernstein_basis are inherited
from PPoly, and neither produces an Akima interpolator.
@ev-br ev-br added PR labels Jun 18, 2014
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d841d20 on ev-br:akima_stub into * on scipy:master*.

@rgommers
Copy link
Member

rgommers commented Jul 5, 2014

Akima1DInterpolator has one __init__ method and three overridden not-implemented methods. Doesn't that indicate that it's inheriting from the wrong class? Maybe inherit from _PPolyBase and add the methods that do make sense instead?

disclaimer: I haven't looked at this set of classes before.

@ev-br
Copy link
Member Author

ev-br commented Jul 7, 2014

An upshot of inheriting from PPoly is inheriting useful methods, integrate, derivative, roots etc, which are not available in _PPolyBase (nor should they).

An alternative approach was taken in a recent refactor of PchipInterpolator, which inherits straight from object and carries around a private BPoly instance. In its current state it is still not very convenient, gh-3742.

I still think encapsulation is a better way to go here, and this PR is simply a bit of band-aid to attach until we've time to properly fix both Akima and PCHIP. Which IMO requires a bit of thought on how to handle (or not handle) axis arguments and out-of-bounds behavior.

@rgommers
Copy link
Member

I understand that you need those methods, that's what I was referring to with "add the methods that do make sense". In general if you have the choice between inheriting 6 methods and then raising NotImplementedError for 3 of them, or inheriting none and adding 3 via def integrate(self, ...): return the_other_integrate(...), the latter is cleaner.

If this is a band-aid, then it doesn't matter too much though.

@rgommers
Copy link
Member

This is still an improvement, so merging.

rgommers added a commit that referenced this pull request Jul 13, 2014
stub out inherited methods of Akima1DInterpolator
@rgommers rgommers merged commit 266e268 into scipy:master Jul 13, 2014
@rgommers rgommers added this to the 0.15.0 milestone Jul 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants