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

Handle non-interpolating dimensions and improve in-place interpolation #45

Merged
merged 9 commits into from Jul 18, 2015

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 4, 2015

See the comments at the top of the last two commits. (Built on top of #44, so the first 4 commits are redundant.)

In the same way as a matrix can be viewed as a collection of column
vectors, this allows a matrix to be viewed as a collection of
column-interpolants. Compared to just using Constant, the main
motivation for a new type is for specifying the number of gradient
components.
This allows evaluation all the way to the boundary, preserving the
on-grid values.
@timholy
Copy link
Member Author

timholy commented Jul 4, 2015

How do you feel about the last commit? (The InPlaceQ commit.) This provides the same behavior as BCnil in Grid. I found I need this because I have a lot of test-suite code that compares against ground-truth quadratic functions, and so something that matches to within machine precision is a huge advantage. However, if you think this is too specialized for Interpolations, that last patch could live in my own code tree.

@timholy
Copy link
Member Author

timholy commented Jul 10, 2015

Any more thoughts about this series of PRs? It's not urgent to get them merged, but I am starting to use this functionality heavily in my own code tree.

@tomasaschan
Copy link
Contributor

Sorry, meant to come back to this earlier but other things came in the way.

As can be expected when you're the committer, the work is splendid. It works really well :)

I'm not entirely happy with the need to construct dimspecs as e.g. Tuple{BSpline(Quadratic(Flat)), BSpline(Linear)} rather than just (BSpline(Quadratic(Flat), BSpline(Linear)). The only way I can think of that would fix that, would be skip the passing of types altogether, and start passing instances instead, i.e. interpolate(A, BSpline(Linear(), OnGrid()) that could be coupled with interpolate(A, (BSpline(Linear()), BSpline(Quadratic(Flat()))), OnGrid()) and now it starts to look like Lisp... Not quite happy with that either. Which do you prefer? Do you see a way to get my desired structure (with parentheses only where needed for grouping)?

There are also a few constructor methods missing, but they can be added after merging these PR's without any problems, so they don't have to be blocking - but for example, I'd like to see interpolate(A, BSpline(Linear, Quadratic(Flat)), OnGrid) as shorthand for giving a Tuple{BSpline(Linear), BSpline(Quadratic(Flat))} as the interpolation type argument. Doing this might be controversial, though, since it would mean that a method of BSpline doesn't return an instance of a BSpline (but rather a tuple of them)... I can take a stab at adding the ones I feel are missing after we merge these PR's.

With regards to InPlace and InPlaceQ, I must admit that I don't fully understand the mathematics behind them from just reading the code. I don't doubt that they're useful, but my confusion indicates that they can probably, at least, be better named. What are the mathematical boundary conditions that they implement? What behavior does that describe? Eventually, this information should probably be added to doc/latex/Interpolations.tex...

@timholy
Copy link
Member Author

timholy commented Jul 11, 2015

I'm not entirely happy with the need to construct dimspecs as e.g. Tuple{BSpline(Quadratic(Flat)), BSpline(Linear)} rather than just (BSpline(Quadratic(Flat), BSpline(Linear)).

Understood. Eventually Tuple{A,B} seems likely to be written as {A,B}, but certainly not for 0.4 (since its old meaning as Any[] needs one cycle of deprecation). One potential way forward is illustrated here:

julia> immutable Foo{T} end

julia> f = Foo{Tuple{Int,Float64}}()
Foo{Tuple{Int64,Float64}}()

julia> f = Foo{(Int,Float64)}()
ERROR: TypeError: apply_type: in Foo, expected Type{T}, got Tuple{DataType,DataType}

julia> Foo(t::Tuple) = Foo{Base.to_tuple_type(t)}()
Foo{T}

julia> f = Foo((Int,Float64))
Foo{Tuple{Int64,Float64}}()

This may also partially address your constructor issue. Assuming you want to do this (rather than just wait for {} to mean Tuple{}), I can add this.

I will also write something for the LaTeX document; agreed that this is important. I'll try to get that done this weekend.

@timholy
Copy link
Member Author

timholy commented Jul 11, 2015

I should add that we still need to preserve the "proper" Tuple{} forms, because the (A,B) form is not type-inferrable:

julia> foo{T}(::Type{T}) = Foo{T}()
foo (generic function with 1 method)

julia> foo{T<:Tuple}(t::T) = Foo(t)
foo (generic function with 2 methods)

julia> function myfunc1()
           f = foo(Tuple{Int,Float64})
           nothing
       end
myfunc1 (generic function with 1 method)

julia> function myfunc2()
           f = foo((Int,Float64))
           nothing
       end
myfunc2 (generic function with 1 method)

julia> @code_warntype myfunc1()
Variables:
  f::Foo{Tuple{Int64,Float64}}

Body:
  begin  # none, line 2:
      f = $(Expr(:new, Foo{Tuple{Int64,Float64}})) # line 3:
      return nothing
  end::Void

julia> @code_warntype myfunc2()
Variables:
  f::Foo{T}

Body:
  begin  # none, line 2:
      GenSym(0) = (top(tuple))(Int,Float64)::Tuple{DataType,DataType}
      f = call((top(apply_type))(Foo,((top(getfield))(Base,:to_tuple_type))(GenSym(0))::Union{Type{_<:Tuple{Any,Any}},Tuple{DataType,DataType}})::Type{_<:Foo{T}})::Foo{T} # line 3:
      return nothing
  end::Void

(too bad the red syntax highlighting doesn't show up here).

@tomasaschan
Copy link
Contributor

Well, if we can't get proper type inference without constructing tuples using Tuple{}, then I'll have to live with that :) It might be a good idea to deprecate the mix-and-match style we have now for just plain parametric types everywhere - i.e. we write also BSpline{Quadratic{Line}} rather than BSpline(Quadratic(Line)). Today, the latter is a function that returns the former, but the style step from BSpline{Linear} to Tuple{BSpline{Linear}, BSpline{Quadratic{Flat}}} is much smaller. That can be handled separately, though, so no need to fix it right now (we probably want to deprecate the old functions first anyway). Given that, I don't think there's a need to change the current syntax for dimspecs.

Looking forward to the math docs! :)

@timholy
Copy link
Member Author

timholy commented Jul 13, 2015

OK, I've added the math docs, as well as our first test for multivalued interpolation. (The latter should be useful as a reference for anyone who wants to implement the required interface.)

@timholy
Copy link
Member Author

timholy commented Jul 13, 2015

Oh, and can I ask you to re-run pdflatex on the file? I don't have biber, and the installation would eat up a lot of my little remaining hard-drive space 😦. (The curse of using an SSD on a machine that you have a lot of data stored on...)

@timholy
Copy link
Member Author

timholy commented Jul 18, 2015

Bump. Barring any complaints, I will merge this shortly.

We'll also need a big doc update (which I think can be done after this gets merged). Would you prefer to keep most of the documentation in the IJulia notebook, and make the README minimalistic? In a sense the README could be a summary of what's available, now that

julia> using Interpolations
julia> ?Interpolations

dumps the README contents. So I've begun to think that it might be good to keep the README as an "API reminder" for people who already understand the package, and the IJulia notebook as an introduction to the package.

@tomasaschan
Copy link
Contributor

Fine with me :)

As far as documentation goes, I just opened #46.

timholy added a commit that referenced this pull request Jul 18, 2015
Handle non-interpolating dimensions and improve in-place interpolation
@timholy timholy merged commit ba01661 into master Jul 18, 2015
@timholy timholy deleted the teh/moretypes branch July 18, 2015 13:03
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

2 participants