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

Convenience constructors #214

Merged
merged 10 commits into from
Jul 21, 2018
Merged

Conversation

chiyahn
Copy link
Contributor

@chiyahn chiyahn commented Jul 19, 2018

Here I have added some convenience constructors that can be used to create interpolation objects in an accessible way as suggested in #206 (comment). Note that they support multidimensional cases as well; see unit tests located at test/convenience-constructors.jl for some examples.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@lstagner
Copy link

This is great. However, I think it would be better to make the extrapolation behavior a keyword argument. Something like

LinearInterpolation(ranges::NTuple{N,T}, vs; bc = Interpolations.Throw()) where {N,T <: Range} = extrapolate(scale(interpolate(vs, BSpline(Linear()), OnGrid()), ranges...),  bc)

chiyahn added 2 commits July 19, 2018 17:35

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@jlperla
Copy link

jlperla commented Jul 19, 2018

@lstagner I agree with you. Though I think that in general the approach should be to make the basics easy, and let people stick with the full interface when they want to do anything fancy.

But with that logic, I do think that throwing vs. non-throwing is sufficiently important to be made easy... having the throwing adds about a 1/4 performance overhear from our quick tests, so it would be very nice to get things working with default Throw to ensure hidden bugs don't creep in, and then easily turn them off when you want to tweak the performance.

@jlperla
Copy link

jlperla commented Jul 19, 2018

@ChrisRackauckas @sglyon Here are the convenience constructors for feedback. If you have any early comments tell us and we can tweak it before a merge is appropriate. This is probably the last of the Interpolation.jl tweaks we are planning for now.

@jlperla
Copy link

jlperla commented Jul 20, 2018

@chiyahn Do you think this is ready to go if there is a little documentation? If so, you could add in a few very simple examples below the General Usage in the README.md that shows

  • LinearInterpolation with 1D with a regular grid
  • LinearInterpolation in 1D with an irregular grid
  • CubicSpline in 1D with a regular grid
  • LinearInterpolation with 2D and a regular grid?

I think that is about it, we can keep the convenience constructor examples very simple and tell people to use the full API otherwise?

Sorry, something went wrong.

@tomasaschan
Copy link
Contributor

tomasaschan commented Jul 20, 2018

Maybe also in the examples pick one and show how to configure extrapolation. Maybe also have one test for that.

The tests could probably be expressed more concisely by, instead of actually interpolating stuff, just creating one interpolation object using the convenience constructor and one using the full API, and then verifying that they have the same type. That way, the tests also act as extra documentation for how to go between the two representations.

But with that, all green lights from me!

@sglyon
Copy link
Member

sglyon commented Jul 20, 2018

One other question would be whether or not we want to control boundary conditions on Cubic splines? Right now I believe they are set to always use Line, but we could make that a keyword argument if we wanted .

@jlperla
Copy link

jlperla commented Jul 20, 2018

@tlycken All your suggests sound great to me.

@sglyon is your thinking that we would do something like

CubicSplineInterpolation(ranges::NTuple{N,T}, vs; bc = Interpolations.Line(), extrapolation_bc = Interpolations.Throw()) where {N,T <: Range} = extrapolate(scale(interpolate(vs, BSpline(Cubic(bc)), OnGrid()), ranges...), extrapolation_bc)

Do you like those names for the keywords?

If so, I think it is a reasonable addition. At that point, we are probably at the point where if people want additional features they should use the full interface?

@jlperla
Copy link

jlperla commented Jul 20, 2018

@sglyon @lstagner @tlycken @ChrisRackauckas : Does anyone have any other comments or todos on this PR? I believe this implements all of the feedback to date?

If people are generally happy with it, then maybe we could start a ticking clock on the PR, with a plan to tag a release of the library combining these constructors and the () access?

@chiyahn
Copy link
Contributor Author

chiyahn commented Jul 20, 2018

One more last thing -- added an extra parameter for boundary conditions in CubicSplineInterpolation as @sglyon suggested 😃

@sglyon
Copy link
Member

sglyon commented Jul 21, 2018

This looks good to me!

Thanks.

Starting a clock now. Will merge within 24 hours unless I see something here

@sglyon sglyon merged commit 2633025 into JuliaMath:master Jul 21, 2018
@lstagner
Copy link

The quick start guide needs to be updated to use the function call syntax.

@chiyahn chiyahn deleted the convenience-constructors branch July 23, 2018 19:12
@chiyahn chiyahn mentioned this pull request Jul 23, 2018
@chiyahn
Copy link
Contributor Author

chiyahn commented Jul 23, 2018

That's right; here I made a PR for revision: #216. Thanks for the reminder!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants