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

Added tests for Static. #69

Merged
merged 12 commits into from
Sep 9, 2020
Merged

Added tests for Static. #69

merged 12 commits into from
Sep 9, 2020

Conversation

chriselrod
Copy link
Collaborator

@Tokazama
This is the code for Static. The OptionallyStaticUnitRange could probably be cleaned up further.

Currently known_* functions still return nothing or values, but I added static_* that use these and return either Static numbers (if known_ returns a value) or values (if they return nothing). These will likely often be more convenient to use, and seem strictly more powerful.

@Tokazama
Copy link
Member

Tokazama commented Sep 9, 2020

This is great! The static_* methods seem like a nice way to ensure that static numbers are only used when asking for them.

Do you think it would be useful to support other integer types? If so I'm thinking of something that's still limited to types defined in base like one of the following

struct Static{N} <: Integer
    function Static{N}()  where {N}
        @assert isprimitivetype(typeof(N)) "$N is not a primitive type"
        return new{N}()
    end
end
struct Static{N} <: Integer
    function Static{N}()  where {N}
        @assert isa(N, Base.BitIntegerType) "$N is not a primitive type"
        return new{N}()
    end
end

@chriselrod
Copy link
Collaborator Author

chriselrod commented Sep 9, 2020

The reason I restricted it to Int was because of defining all the methods like Base.:(::Static{0}, x) = Static{0}().
I also realize now I should also add more generic methods (i.e., where x is untyped like in that example).

I also have a lot of trust that dispatch will be type stable.
But branches should be fine, too. If we go for allowing more integer types, we can use branchy definitions such as:

@inline function Base.:(*)(x::Static{M}, y) where {M}
    if iszero(M)
        x
    elseif isone(M)
        y
    else
        M * y
    end
end

and I think it's unlikely inference will fail.

@timholy
Copy link
Member

timholy commented Sep 9, 2020

Allowing other Integer subtypes risks increasing your compile-time latency and likelihood of difficult-to-track-down bugs; try debugging why

julia> Val(3)
Val{3}()

julia> Val(Int16(3))
Val{3}()

julia> typeof(Val(3))
Val{3}

julia> typeof(Val(Int16(3)))
Val{3}

aren't the same type. Unless there's a compelling reason, diversifying the Integer types seems like a lot of pain for little or no benefit.

@chriselrod
Copy link
Collaborator Author

I'll revert adding support for more integer types.

@Tokazama
Copy link
Member

Tokazama commented Sep 9, 2020

I'm totally on board with just supporting Int. I just wanted to ensure it wouldn't be too restrictive for some future use.

@chriselrod
Copy link
Collaborator Author

chriselrod commented Sep 9, 2020

Just realized I also accidentally created a second branch called "static" where I added support for the integer types:
https://github.com/chriselrod/ArrayInterface.jl/tree/static
That one at chriselrod/ArrayInterface instead of at SciML/ArrayInterface like this PR.

So I can just leave this one as is, or just apply the @eval style used there here.

An advantage of the branchy style worth considering is that it is less prone to ambiguity errors.

@Tokazama
Copy link
Member

Tokazama commented Sep 9, 2020

An advantage of the branchy style worth considering is that it is less prone to ambiguity errors.

It might be good for us to start using detect_ambiguities to weed these things out. If it doesn't affect performance and reduces ambiguities the branching style may be preferable, but there is something beautiful in the simplicity of all those one line methods.

@chriselrod
Copy link
Collaborator Author

I have noticed that using a lot of @eval loops can severely impact package load time, so I think it is normally preferable to keep the number of methods relatively small.

We have 36 definitions in the +/-/* loop.
The main savings we get is that, by not defining specific ::Static{0} or ::Static{1} definitions, we don't need to define any methods for the various combinations to avoid ambiguities.

I added

@test isempty(detect_ambiguities(ArrayInterface))
@test isempty(detect_unbound_args(ArrayInterface))

@timholy
Copy link
Member

timholy commented Sep 9, 2020

Can also use https://github.com/JuliaTesting/Aqua.jl

@Tokazama
Copy link
Member

Tokazama commented Sep 9, 2020

Can also use https://github.com/JuliaTesting/Aqua.jl

That's really cool! I should be using that.

@chriselrod
Copy link
Collaborator Author

chriselrod commented Sep 9, 2020

Sure, now using Aqua.jl.
Apparently it catches ambiguities (in Julia 1.2) that were "missed" before:

5 ambiguities found

Ambiguity #1
promote_rule(::Type{T}, ::Type{#s16} where #s16<:ArrayInterface.Static) where T>:Nothing in ArrayInterface at /home/travis/build/SciML/ArrayInterface.jl/src/static.jl:28
promote_rule(::Type{Any}, ::Type{#s75} where #s75) in Base at promotion.jl:235

Ambiguity #2
promote_rule(::Type{#s16} where #s16<:ArrayInterface.Static, ::Type{T}) where T in ArrayInterface at /home/travis/build/SciML/ArrayInterface.jl/src/static.jl:19
promote_rule(::Type{#s75} where #s75, ::Type{Any}) in Base at promotion.jl:236

Ambiguity #3
promote_rule(::Type{T}, ::Type{#s16} where #s16<:ArrayInterface.Static) where T>:Union{Missing, Nothing} in ArrayInterface at /home/travis/build/SciML/ArrayInterface.jl/src/static.jl:27
promote_rule(::Type{Any}, ::Type{#s75} where #s75) in Base at promotion.jl:235

Ambiguity #4
promote_rule(::Type{T}, ::Type{#s16} where #s16<:ArrayInterface.Static) where T>:Missing in ArrayInterface at /home/travis/build/SciML/ArrayInterface.jl/src/static.jl:29
promote_rule(::Type{Any}, ::Type{#s75} where #s75) in Base at promotion.jl:235

Ambiguity #5
promote_rule(::Type{T}, ::Type{#s15} where #s15<:ArrayInterface.Static) where T in ArrayInterface at /home/travis/build/SciML/ArrayInterface.jl/src/static.jl:20
promote_rule(::Type{Any}, ::Type{#s75} where #s75) in Base at promotion.jl:235

The first definition is strictly more specific in each case.

@timholy
Copy link
Member

timholy commented Sep 9, 2020

@johnnychen94 introduced me to Aqua, I haven't yet used it.

@chriselrod, interesting. What does the T >: Nothing accomplish? Could you declare these

promote_rule(::Type{Nothing}, ::Type{S}) where S<:Static = ...

@chriselrod
Copy link
Collaborator Author

chriselrod commented Sep 9, 2020

What does the T >: Nothing accomplish?

Resolves a type ambiguity on master.
I had 0 ambiguities in Julia 1.2 and 1.5, but over 300 in Julia 1.6.
It took quite the wack-a-mole to get it down to 0.

I imagine I didn't do it particularly efficiently.

Could you declare these

I already had definitions like

promote_rule(::Type{Nothing}, ::Type{<:Static}) = ...

Is there a difference in specificity?
I still have the same ambiguity errors.

@timholy
Copy link
Member

timholy commented Sep 9, 2020

Try rebuilding master? An ambiguity-related commit just got reverted within the last hour or so.

@chriselrod
Copy link
Collaborator Author

JuliaLang/julia#37484

To what extant is it better to err on the side of conservative here?

Anyway, the two former ones seem serious enough that is worth to revert this to get back to the "baseline" and then eventually put it back when the problems have been fixed.

Sounds like it would be better long term to be strict.

@chriselrod
Copy link
Collaborator Author

That said, no errors on Travis on 1.2, 1.5, or master now.
Presumably Travis hasn't updated master to reflect the reversion of the ambiguity commit yet (but I've tested locally before and after to confirm).

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

I didn't check in gory detail, but LGTM

src/ranges.jl Outdated Show resolved Hide resolved
src/ranges.jl Outdated Show resolved Hide resolved
@Tokazama
Copy link
Member

Tokazama commented Sep 9, 2020

I think this looks great.

@chriselrod chriselrod merged commit 85e93de into master Sep 9, 2020
@chriselrod chriselrod deleted the static branch October 9, 2020 01:19
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.

3 participants