-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
[MRG] ENH: CubicSpline interpolator #5653
Conversation
c50323d
to
2339e17
Compare
-------- | ||
Akima1DInterpolator | ||
PchipInterpolator | ||
|
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.
Might want to also list PPoly here.
This looks pretty good! I've left some comments based on a quick read (did not check the math yet). If this is supposed to be beginner-friendly, some more commenting of the construction would be nice. A simple example in the docstring would be nice, too. As far as the math is concerned, it is worth spending a little bit of effort to check the numerical stability of the specific implementation. |
Very useful notes, Evgeni. I addressed most of the things. What about NaN's handling? As example I'm thinking about the plot of derivatives 0, 1, 2, 3 to show that the continuity property breaks only for the third derivative. |
NaN handling: I'd suggest to 1) add a test that data with nans raises, and 2) add a note to the docstring. Expect a bunch of pandas users who expect some sort of nan-means-missing-value intelligence :-). |
+1 |
I came up with a way to check continuity of the solution, using 1st order Taylor approximation to compare close values near break points. This way I could sensibly check solution properties with Regarding the first test: one can obtain the same values in Octave using the code provided. |
An example is here. More or less it is ready, so I added [MRG] tag. |
@@ master #5653 diff @@
======================================
Files 235 235
Stmts 43282 43347 +65
Branches 8155 8164 +9
Methods 0 0
======================================
+ Hit 33664 33723 +59
- Partial 2591 2594 +3
- Missed 7027 7030 +3
|
This definitely needs an entry in the release notes. |
I wrote a note to release notes. |
Fixed problems after rebase. |
@ev-br yeah, ok. I reverted changes, look forward to see a new PR from Matthieu |
CI failure is due to the bot complaining about whitespace.
|
7f46390
to
b27c36a
Compare
BTW, gh-5734 in part addresses concerns of @juliantaylor and @argriffing on what is the value added by |
I plan to merge this soonish, unless further comments. |
needs a rebase now |
b27c36a
to
4b7f5fe
Compare
Rebased. |
[MRG] ENH: CubicSpline interpolator
Merged. |
As was discussed in #5637 it can be beneficial to have a simple cubic spline interpolator, which could be easily understand by users inexperienced in splines.
Hope to see some feedback.