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

Mismatch between returned value type and eltype for Float16 data #308

Closed
lcontento opened this issue Mar 22, 2019 · 3 comments
Closed

Mismatch between returned value type and eltype for Float16 data #308

lcontento opened this issue Mar 22, 2019 · 3 comments

Comments

@lcontento
Copy link

When interpolating Float16 data, the interpolator reports its element type as Float64 even if it correctly returns Float16 data when called.

julia> using Interpolations

julia> itp = interpolate(rand(Float16, 3, 3), BSpline(Linear()));

julia> isa(itp, Interpolations.BSplineInterpolation{Float16})
false

julia> eltype(itp)
Float64

julia> itp(Float16(1.1), Float16(1.1))
Float16(0.9077)

It looks to be due to the tweight function defaulting to Float64 and I can apparently solve the problem by specializing tweight for Float16 or changing its default value to be the same type as the input array. Since I am not very familiar with the codebase I am not sure if that's all it should be done.

@tomasaschan
Copy link
Contributor

After we decided to have a functional interface rather than an array one (see e.g. #198, #206, #242), I'm not sure it makes sense to expect eltype to indicate the type returned by interpolating.

If #242 results in actually dropping AbstractArray inheritance, we should probably remove it altogether (this is my preference, but I'm no longer an active user of the project, so that's based solely on design principles and not on actual usage), while if it results in re-defining what the array interface of an interpolation object refers to, it might still, in at least some cases, make sense to have eltype(itp) != typeof(itp(x)).

In other words, I'm not sure this should be changed at all, and if it should, the question of how is probably still not settled.

@lcontento
Copy link
Author

I see, I was mistaken in believing that eltype was referring to the output type. However, I still believe that the current behaviour for Float16 should be changed for consistency's sake. We have that

let A = rand(T, 3, 3), itp = interpolate(A, BSpline(Linear()))
    isa(itp, Interpolations.BSplineInterpolation{T}) && eltype(itp) == T
end

is true for all the basic floating point types T (e.g., T=Float32), so as a user I would expect it to hold true also for T=Float16. By the way, a similar situation also happens with Rationals since tweight is only specialized for Rational{Int} but not for smaller Integer types.

@timholy
Copy link
Member

timholy commented Mar 22, 2019

Until #242 happens, eltype should be the type of the return value when it is "indexed" with Int. It may or may not be true for other index types, and indeed it's often useful that it is not: e.g., passing dual numbers as "indexes" to compute gradients.

Since itp(2, 2) does return a Float16, this is indeed a bug. A fix would be welcome.

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

No branches or pull requests

3 participants