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

ENH: implement Akima interpolation in 1D #3112

Closed
wants to merge 2 commits into from

Conversation

andreas-h
Copy link
Contributor

As promised in #2885, here's my PR for Akima interpolation in 1D. While there are several improvements thinkable, I believe it's already useful as-is and would appreciate inclusion in the upcoming 0.14 release.

@pv
Copy link
Member

pv commented Dec 3, 2013

fill_value doesn't do anything. Would be better to remove both it and the periodic keywords.

Otherwise, looks OK.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 06f435c9a5a4dfd77463a7b7f81f34036e2c5fce on andreas-h:Akima1D into * on scipy:master*.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling dfcbcb9469f816bffc27e92fb86e46358b57d945 on andreas-h:Akima1D into f86894e on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling dfcbcb9469f816bffc27e92fb86e46358b57d945 on andreas-h:Akima1D into f86894e on scipy:master.

coeff[1] = c
coeff[0] = d

PPoly.__init__(self, coeff, x, extrapolate=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a super call would play better with subclassing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, mea culpa
I think you want super(Akima1D, self).__(coeff, x)

@ev-br
Copy link
Member

ev-br commented Dec 3, 2013

Periodic boundary conditions: https://gist.github.com/ev-br/7110020

@andreas-h
Copy link
Contributor Author

Thanks for the pointer to your code. If you want me to, I can prepare a seperate PR to include periodic boundary conditions for PPoly.

@andreas-h
Copy link
Contributor Author

Updated subclassing to use super() call.

@ev-br
Copy link
Member

ev-br commented Dec 3, 2013

Feel free to do whatever you feel like with these lines of code.
We had a discussion with Pauli about what to do with out-of-bounds behavior in his PPoly PR. I think the decision was to leave them to a user.
Given that periodic boundary conditions are literally five lines of code, I'm not sure it's worth inclusion into scipy. And if we do have PBCs, why not also antiperiodic (DMFT people use antiperiodic Akimas), mirror-symmetric (b-splines in signal) etc.

An optional enhancement would be to clarify the relation of the Akima splines and pchip.

Irrespective of this, I'm +1 on this PR.

@pv
Copy link
Member

pv commented Dec 3, 2013

One thing needs to be added: it should support multidimensional y-arrays, not only 1D. This feature is very often needed.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f91f7c2 on andreas-h:Akima1D into 10d88c4 on scipy:master.

@andreas-h
Copy link
Contributor Author

I agree multidimensional y arrays would be useful. The necessary machinery is already there in the _Interpolator1D class. However, when I try to inherit from both PPoly and _Interpolator1D, I get into some inheritance problems I've never encountered before:

TypeError: Error when calling the metaclass bases
multiple bases have instance lay-out conflict

I'm not sure where this comes from, but it might be because the __slots__ are defined in both parent classes. Any ideas how I could go about this?

As an alternative approach to this, I could go a different route: implement the Akima algorithm in a helper class _Akima1D and then extent the interp1d class to support kind=akima. What do you think?

@pv
Copy link
Member

pv commented Dec 9, 2013

It would probably be enough to support n-d so that the interpolation axis is the first one. The PPoly/BPoly can handle this, so in Akima the only thing to do is to make sure the coefficient computation is vectorized accordingly.

@pv
Copy link
Member

pv commented Dec 13, 2013

The current code in __init__ almost handles the n-d case, too. It seems the only thing needed is to change the coeff array shape to (4,) + y.shape, and to add some n-d tests.

If the axis transposition is a feature that could be useful, it should be added in PPoly. There's no need to inherit from _Interpolator1D here, as it's only a helper class whose main purpose is to make axis transpositions etc. easier.

@andreas-h
Copy link
Contributor Author

Done; Akima now doesn't inherit _Interpolator1D any more, and supports n-d y.

Is there a more elegant way to do the "broadcasting" I'm doing in l. 1433-1435? It feels hackish what I'm doing there.

@andreas-h
Copy link
Contributor Author

Sorry, just realized I amended my changes to the last commit and did a --force push. Seems like I'm still doing stupid things with git ... :-/

# determine slopes between breakpoints
m = np.empty((x.size + 3, ) + y.shape[1:])
dx = np.diff(x)
for i in range(y.ndim - 1):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equivalent to dx = dx[(slice(None),) + (None,)*(y.ndim-1)] ?
If so, better not use as_strided

@pv
Copy link
Member

pv commented Feb 8, 2014

Force-pushing to pull requests is also OK

@andreas-h
Copy link
Contributor Author

If this is okay, can we get it into 0.14?

@pv
Copy link
Member

pv commented Feb 18, 2014

I'll (i) rebase, (ii) rename to AkimaInterpolator, and (iii) move the implementation to _monotone.py together with pchip

@ev-br
Copy link
Member

ev-br commented Feb 18, 2014

Could you also add a 'see also' crosslinks to both docstrings.

@pv
Copy link
Member

pv commented Feb 18, 2014

Ok, actually this has to wait for tomorrow from my side, got to go...
But looks ready to go to me.

y : ndarray, shape (m, ...)
N-D array of real values. The length of *y* along the first axis must
be equal to the length of *x*.
axis : int, optional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left over? The init below does not seem to have an axis argument (I realize I'm a little late to the party...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True ... will force-push fix

@ev-br
Copy link
Member

ev-br commented Feb 18, 2014

(this is by no means intended to block merging)

Can you comment on this: https://gist.github.com/ev-br/9081651

I've to admit I did not read Akima's paper, but I somehow had an impression that akima splines were supposed to not overshoot?

@andreas-h
Copy link
Contributor Author

interesting. I'll need to check, hopefully tomorrow. I know Akima's are supposed not to overshoot, but I don't remember if they're guaranteed not to do so. Have to go now, but will come back to this very soon.

@ev-br
Copy link
Member

ev-br commented Feb 19, 2014

Handling of arbitrary axis can be done in a way similar to pchip: basically a single private helper is enough to make the behavior consistent with that of polyint-based interpolators. Which can be done now or left as an optional enhancement.

@pv pv added the PR label Feb 19, 2014
@pv
Copy link
Member

pv commented Feb 19, 2014

I think the implementation here is correct. I get the same overshooting curve from other Akima implementations, so it's a property of the algorithm.

@pv
Copy link
Member

pv commented Feb 19, 2014

Thanks a lot Andreas! PR merged in 358d1aa.

@pv pv closed this Feb 19, 2014
@pv pv added this to the 0.14.0 milestone Feb 19, 2014
@andreas-h andreas-h deleted the Akima1D branch February 24, 2014 10:44
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

5 participants