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

Allow manual override of return type for fillvalue #72

Merged
merged 1 commit into from
Oct 3, 2015

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 27, 2015

I'm playing with ForwardDiff as a way of computing gradients, and needed this to handle the fact that sometimes the same FilledExtrapolation object gets called with two different types of indexes (Float64 and GradientNumber{Float64,...}). I added these lines (the last one being the relevant one here) to my own code:

# Some necessary ForwardDiff extensions
Base.real(v::ForwardDiff.GradientNumber) = real(v.value)
Base.ceil(::Type{Int}, v::ForwardDiff.GradientNumber)  = ceil(Int, v.value)
Base.floor(::Type{Int}, v::ForwardDiff.GradientNumber) = floor(Int, v.value)
# Type-stability in gradient computation
@generated function Interpolations.fillvalue{T<:ForwardDiff.GradientNumber}(val, args::T...)
    :(ForwardDiff.GradientNumber(val, args[1].partials.data))
end

and now ForwardDiff works fine.

@tomasaschan
Copy link
Contributor

I thought this was the purpose of exporting the FilledExtrapolation constructor, or am I misunderstanding something? Can all the goals of these two things be accomplished with just one of them?

@timholy
Copy link
Member Author

timholy commented Sep 28, 2015

It was, but that's a static mechanism. What if you call the same FilledExtrapolation object with two different types of indexes? For example, itp[-3.2] might give you NaN, but itp[dual(-3.2,1.0)] (with the same itp) should give you dual(NaN, 1.0).

@timholy
Copy link
Member Author

timholy commented Sep 28, 2015

Though I suppose we could use the promote_op mechanism instead?

@tomasaschan
Copy link
Contributor

My question was not as much "do we need this?" as "do we need both, or can this replace the other?" but if both are valuable I see no reason not to keep them both anyway. So it was more a curious question than criticism :)

But using promote_op sounds like a good idea to me - otherwise we'll basically re-implement a mechanism for the user to do the same thing, but hard-coded (and thus error-prone). Perhaps we can accomplish something similar to how we calculate T when creating B-spline interpolation objects, and/or the array type for gradient (see below, soon to be included in the PR for extrapolation behaviors)?

@generated function gradient{T,N}(itp::AbstractInterpolation{T,N}, xs...)
    n = count_interp_dims(itp, N)
    Tg = promote_type(T, [x <: AbstractArray ? eltype(x) : x for x in xs]...)
    :(gradient!(Array($Tg,$n), itp, xs...))
end

Regardless of what the actual solution ends up being, I think we should try to a) make sure that we are compatible with the promotion mechanisms in base Julia as much as possible, and b) make the user-interface as seamless ass possible. Leveraging promote_op to accomplish a) can probably help with b), so that's a good place to start :)

@tomasaschan
Copy link
Contributor

Btw, the test failure should be fixed once #73 is merged.

@timholy timholy force-pushed the teh/typestability_fillvalue branch 2 times, most recently from 72240c8 to 23b6007 Compare September 30, 2015 20:37
@timholy
Copy link
Member Author

timholy commented Sep 30, 2015

OK, see if you like this better (I do).

I deleted the docs for FilledExtrapolation, since they focused on controlling the return type (which has proven to be of very limited utility). Should we un-export the type, or leave it?

@@ -27,6 +27,8 @@ etpf = @inferred(extrapolate(itpg, NaN))
@test_throws BoundsError etpf[2.5,2]
@test_throws ErrorException etpf[2.5,2,1] # this will probably become a BoundsError someday

@inferred(getindex(etpf, dual(-2.5,1)))

etpf = @inferred(FilledExtrapolation(itpg, 'x'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this initialization be done some other way?

Copy link
Member Author

Choose a reason for hiding this comment

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

This test can probably just be dropped, since it was really testing explicit control.

@tomasaschan
Copy link
Contributor

Should we un-export the type, or leave it?

Since exporting the type was mainly to make the type-controlling constructor available, I think we should probably un-export it. (That's a very simple breaking change to work around, should there be anyone who has already started using it - just say FilledExtrapolation = Interpolations.FilledExtrapolation and you're done...) Un-exporting also gives symmetry to other types, where almost no types - except the different configuration "flags" - are exported from the rest of the library.

@timholy timholy force-pushed the teh/typestability_fillvalue branch from 23b6007 to d7d84bc Compare October 1, 2015 11:04
@timholy
Copy link
Member Author

timholy commented Oct 1, 2015

Thanks for the review (and catches). Should be addressed now. There's some weird SSL error on nightly (I restarted, and that didn't fix it), but I doubt it reflects a problem with this PR.

@timholy
Copy link
Member Author

timholy commented Oct 1, 2015

All package PRs are failing this way today.

@tomasaschan
Copy link
Contributor

All package PRs are failing this way today.

😿

@timholy
Copy link
Member Author

timholy commented Oct 2, 2015

Merge anyway? (I can't, it won't let me.)

@tomasaschan
Copy link
Contributor

I assume this is since JuliaLang/julia#11196 merged, and that it'll keep being broken until the regressions are fixed. I also assume this might take a while..:

I'll disable the build on nightly for now; after that, restarting the build should make it pass and allow merging (I'm using Github's semi-new "protect branch" feature on master, to prevent merging before checks pass, but it probably doesn't make sense to require that everything passes on a very unstable Julia master...).

@tomasaschan
Copy link
Contributor

See #75.

@tomasaschan
Copy link
Contributor

Rebasing this onto current master should fix the build (by not trying on 0.5...).

This should promote greater type-stability
@timholy timholy force-pushed the teh/typestability_fillvalue branch from 7baad90 to 67fd8f0 Compare October 3, 2015 10:28
timholy added a commit that referenced this pull request Oct 3, 2015
Allow manual override of return type for fillvalue
@timholy timholy merged commit 2ba7253 into master Oct 3, 2015
@timholy timholy deleted the teh/typestability_fillvalue branch October 3, 2015 10:30
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.

2 participants