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

Make fallback similar methods less specific in axes #1176

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Jul 13, 2023

The idea of this PR is to make the fallback similar method less specific in the axes, to make it easier for packages to extend similar for custom array types without encountering ambiguities.

The way the fallback similar method is defined currently makes it tricky for custom array types to extend similar without running into ambiguities. For example, an array type defined as:

julia> struct CustomArray{T} <: AbstractVector{T}
                       sz :: Int
                   end

julia> Base.size(C::CustomArray) = (C.sz,)

julia> Base.getindex(C::CustomArray{T}, i::Int) where {T} = T(i)

julia> Base.similar(C::CustomArray, ::Type{T}, ax::Tuple{Vararg{Int}}) where {T} =
                       Array{T}(undef, ax)

julia> function Base.similar(C::CustomArray, ::Type{T}, ax::Tuple{Vararg{Union{Int, SOneTo, Base.OneTo{Int}}}}) where {T}
                       sz = last.(ax)
                       Array{T}(undef, sz)
                   end

julia> c = CustomArray{Int}(4)
4-element CustomArray{Int64}:
 1
 2
 3
 4

faces the error

julia> similar(c, Float64, (SOneTo(2), Base.OneTo(3)))
ERROR: MethodError: similar(::CustomArray{Int64}, ::Type{Float64}, ::Tuple{SOneTo{2}, Base.OneTo{Int64}}) is ambiguous.

Candidates:
  similar(A::AbstractArray, ::Type{T}, shape::Union{Tuple{SOneTo, Vararg{Union{Integer, Base.OneTo, SOneTo}}}, Tuple{Union{Integer, Base.OneTo}, SOneTo, Vararg{Union{Integer, Base.OneTo, SOneTo}}}, Tuple{Union{Integer, Base.OneTo}, Union{Integer, Base.OneTo}, SOneTo, Vararg{Union{Integer, Base.OneTo, SOneTo}}}}) where T
    @ StaticArrays ~/.julia/packages/StaticArrays/80e5O/src/abstractarray.jl:149
  similar(C::CustomArray, ::Type{T}, ax::Tuple{Vararg{Union{Base.OneTo{Int64}, Int64, SOneTo}}}) where T
    @ Main REPL[6]:1

Possible fix, define
  similar(::CustomArray, ::Type{T}, ::Union{Tuple{SOneTo, Vararg{Union{Base.OneTo{Int64}, Int64, SOneTo}}}, Tuple{Union{Base.OneTo{Int64}, Int64}, SOneTo, Vararg{Union{Base.OneTo{Int64}, Int64, SOneTo}}}, Tuple{Union{Base.OneTo{Int64}, Int64}, Union{Base.OneTo{Int64}, Int64}, SOneTo, Vararg{Union{Base.OneTo{Int64}, Int64, SOneTo}}}}) where T

Stacktrace:
 [1] top-level scope
   @ REPL[8]:1

After this PR,

julia> similar(c, Float64, (SOneTo(2), Base.OneTo(3)))
2×3 Matrix{Float64}:
 0.0          6.9398e-310  6.9398e-310
 6.9398e-310  0.0          6.9398e-310

This definition of similar feels a bit icky, as

julia> Tuple{} <: StaticArrays.HeterogeneousShapeTuple
true

however,

julia> Tuple{} <: Tuple{Vararg{Int}} <: StaticArrays.HeterogeneousShapeTuple
true

and Base defines similar for Tuple{Vararg{Int}}, which is strictly more specific. This makes similar(::AbstractArray, ::Tuple{}) call the Base method.

This PR also makes something like this work:

julia> similar(zeros(), (1,1,1,SOneTo(1)))
1×1×1×1 Array{Float64, 4}:
[:, :, 1, 1] =
 1.0e-323

where the SOneTo isn't present in the first three arguments.

@mateuszbaran
Copy link
Collaborator

We had something similar before #823 and it was changed to avoid invalidations. Have you checked if it doesn't re-introduce them?

@jishnub
Copy link
Member Author

jishnub commented Jul 17, 2023

The invalidation check passed, so this PR doesn't add to invalidations already present on master, at least on recent versions of Julia

@mateuszbaran
Copy link
Collaborator

I see, Julia must've become better at avoiding invalidations.

@mateuszbaran mateuszbaran merged commit 6bd6f52 into JuliaArrays:master Jul 20, 2023
27 checks passed
@jishnub jishnub deleted the similardisambiguate branch July 20, 2023 08:48
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