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

RFC: possible rework of element type and inferrability #578

Open
timholy opened this issue Jan 28, 2024 · 2 comments
Open

RFC: possible rework of element type and inferrability #578

timholy opened this issue Jan 28, 2024 · 2 comments

Comments

@timholy
Copy link
Member

timholy commented Jan 28, 2024

I've seen cases that make me think that we may need to redesign how construction and position-evaluation compute types. To be clear, I suspect that I am responsible for the current situation, so any weaknesses are my fault. This interacts quite closely with #242 and perhaps it would be best to address both together. See also #359.

There is some ambiguity about what "element type" means for a container like an AbstractInterpolation. At one point, I think the clearest statement came from Jeff Bezanson, which is that the eltype should be the type of object returned when evaluated at integer positions, much as how getindex works on AbstractArrays. However, evaluating at other positions (or with positions encoded by non-integer types) may return objects of different type, because the mathematics performed may be different. One strong possibility is that if we implement #242, then we should make eltype(::AbstractInterpolation) a MethodError.

I've put together a short, simple test that illustrates how I think it should work. Currently, nearly half of these fail.

using Interpolations
using Test

# Non-promotable types that nevertheless have all required arithmetic operations defined.
# Except for the lack of promotion, CoefType ≈ Int and MultType ≈ Float64.
struct CoefType end
struct MultType end
for T in (CoefType, MultType)
    @eval begin
        Base.:*(::$T, ::$T) = MultType()
        Base.:*(::Real, ::$T) = MultType()
        Base.:*(::$T, ::Real) = MultType()
    end
end
Base.:*(::CoefType, ::Int) = CoefType()
Base.:*(::Int, ::CoefType) = CoefType()
Base.:*(::CoefType, ::Int) = CoefType()

Base.:+(::MultType, ::MultType) = MultType()
Base.:+(::CoefType, ::CoefType) = CoefType()

@testset "Non-promotable types" begin
    @test promote_type(CoefType, MultType) === Any   # Any is the only type that "contains" both

    a = fill(CoefType(), 3)
    itp = interpolate(a, BSpline(Linear()))
    # When indexed with integers, linear interpolation does not perform any arithmetic operations.
    # Therefore, the result is of type CoefType.
    @test eltype(itp) === CoefType   # or a MethodError? But if it doesn't error, I think this is what it should return (for Linear)
    @test @inferred(itp(1)) === CoefType()
    # But when evaluated at intermediate positions, linear interpolation does perform arithmetic operations.
    # Therefore, the result is of type MultType.
    @test @inferred(itp(1.5)) === MultType()

    # Extrapolation should behave similarly.
    etp = extrapolate(itp, CoefType())
    @test eltype(etp) === CoefType
    @test @inferred(etp(1)) === CoefType()
    @test @inferred(etp(1.5)) === MultType()
    # Ensure that beyond-the-edge works the same as within-the-edge.
    @test @inferred(etp(4)) === CoefType()
    @test @inferred(etp(4.5)) === MultType()
end

Take-aways

We shouldn't use promote or promote_type. Fundamentally, promotion is about storage ("how do I create a container that can hold both of these objects?"), and that's different from computing the type of object returned by, e.g., (1-f)*a + f*b.

Downstream users, however, may receive new bugs from the lack of assumed real-arithmetic "widening". That is, currently we have

julia> eltype(interpolate([1, 2, 3], BSpline(Linear())))
Float64

because we effectively assume that the created object will be evaluated at Float64-encoded positions. If we change that to Int, as I'm proposing here (because the coefficients are Int), I can imagine that will break a lot of downstream code that relies on eltype(itp) returning something that can accomodate both Int and AbstractFloat evaluation locations. Making eltype throw a MethodError would be one way of preventing more subtle bugs because of this change.

@mkitti
Copy link
Collaborator

mkitti commented Jan 28, 2024

The proposed change is separable from the AbstractArray matter.

The documented definition of eltype is as follows.

Determine the type of the elements generated by iterating a collection of the given type.

As long as iterate and getindex return types are consistent with eltype then I think we can subtype the AbstractArray interface.

Using getindex with non-integer indices has been deprecated for about six years in this package.

julia> itp[1.5]
┌ Warning: `getindex(itp::AbstractInterpolation{T, N}, i::Vararg{Number, N}) where {T, N}` is deprecated, use `itp(i...)` instead.
│   caller = top-level scope at REPL[13]:1
└ @ Core REPL[13]:1
1.5

We should remove methods of getindex for non-integers now. Correspondingly, we may also want to change the eltype to narrower definition.

I would not expect eltype to provide information about the return type from the function call syntax. I would use Base.return_types for that or in Julia 1.11, Base.infer_return_type.

@timholy
Copy link
Member Author

timholy commented Jan 29, 2024

Interesting. There's a lot I like about that. But for a possible counterview: a function is not iterable. Is an interpolant any different from a function?

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

2 participants