Nothing Special   »   [go: up one dir, main page]

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

[MRG] ENH: CubicSpline interpolator #5653

Merged
merged 14 commits into from
Feb 2, 2016
Merged

Conversation

nmayorov
Copy link
Contributor
@nmayorov nmayorov commented Jan 2, 2016

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.

@nmayorov nmayorov force-pushed the cubic_spline branch 2 times, most recently from c50323d to 2339e17 Compare January 2, 2016 13:41
--------
Akima1DInterpolator
PchipInterpolator

Copy link
Member

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.

@ev-br
Copy link
Member
ev-br commented Jan 2, 2016

This looks pretty good! I've left some comments based on a quick read (did not check the math yet).
I'd like to see a test with random values for x and y, and some testing with non-float input. Also handling of input with nans should be fixed in tests (there will be user requests :-).

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.

@ev-br ev-br added enhancement A new feature or improvement scipy.interpolate labels Jan 2, 2016
@nmayorov
Copy link
Contributor Author
nmayorov commented Jan 2, 2016

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.

@ev-br
Copy link
Member
ev-br commented Jan 2, 2016

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 :-).

@ev-br
Copy link
Member
ev-br commented Jan 2, 2016

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.

+1

@nmayorov
Copy link
Contributor Author
nmayorov commented Jan 2, 2016

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 x and y randomly generated.

Regarding the first test: one can obtain the same values in Octave using the code provided.

@nmayorov nmayorov changed the title ENH: CubicSpline interpolator [MRG] ENH: CubicSpline interpolator Jan 3, 2016
@nmayorov
Copy link
Contributor Author
nmayorov commented Jan 3, 2016

An example is here. More or less it is ready, so I added [MRG] tag.

@codecov-io
Copy link
@@            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

Review entire Coverage Diff as of 9ac3b61

Powered by Codecov. Updated on successful CI builds.

@ev-br
Copy link
Member
ev-br commented Jan 3, 2016

This definitely needs an entry in the release notes.

@ev-br ev-br added this to the 0.18.0 milestone Jan 4, 2016
@nmayorov
Copy link
Contributor Author
nmayorov commented Jan 5, 2016

I wrote a note to release notes.

@nmayorov
Copy link
Contributor Author
nmayorov commented Jan 6, 2016

Fixed problems after rebase.

@nmayorov
Copy link
Contributor Author

@ev-br yeah, ok.

I reverted changes, look forward to see a new PR from Matthieu

@argriffing
Copy link
Contributor

CI failure is due to the bot complaining about whitespace.

$ pep8 scipy benchmarks/benchmarks
benchmarks/benchmarks/signal_filtering.py:30:5: E303 too many blank lines (2)
1       E303 too many blank lines (2)

@ev-br
Copy link
Member
ev-br commented Jan 19, 2016

BTW, gh-5734 in part addresses concerns of @juliantaylor and @argriffing on what is the value added by CubicSpline as compared by UnivariateSpline(x, y, s=0). Flexibility in specifying boundary conditions is definitely a useful feature, and adding it to fitpack-based splines is awkward.

@ev-br
Copy link
Member
ev-br commented Jan 27, 2016

I plan to merge this soonish, unless further comments.

@ev-br
Copy link
Member
ev-br commented Feb 1, 2016

needs a rebase now

@nmayorov
Copy link
Contributor Author
nmayorov commented Feb 1, 2016

Rebased.

ev-br added a commit that referenced this pull request Feb 2, 2016
[MRG] ENH: CubicSpline interpolator
@ev-br ev-br merged commit 00dcc96 into scipy:master Feb 2, 2016
@ev-br
Copy link
Member
ev-br commented Feb 2, 2016

Merged.
This is going to be one of the highlights of the release, I'd think. Thanks Nikolay, and keep them coming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement scipy.interpolate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants