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

fix similar() ambiguity with Base #233

Merged
merged 1 commit into from
Jun 23, 2022
Merged

Conversation

aplavin
Copy link
Member

@aplavin aplavin commented Jun 18, 2022

map(f, StructArray) stopped working in 0.6.9 due to a method ambiguity:

julia> s = StructArray(a=rand(4));

julia> map(x -> x, s)
ERROR: MethodError: similar(::StructVector{NamedTuple{(:a,), Tuple{Float64}}, NamedTuple{(:a,), Tuple{Vector{Float64}}}, Int64}, ::Type{NamedTuple{(:a,), Tuple{Float64}}}, ::Tuple{Base.OneTo{Int64}}) is ambiguous. Candidates:
  similar(a::AbstractArray, ::Type{T}, dims::Tuple{Union{Integer, Base.OneTo}, Vararg{Union{Integer, Base.OneTo}}}) where T in Base at abstractarray.jl:803
  similar(s::StructArray, S::Type, sz::Tuple{Union{Integer, AbstractUnitRange}, Vararg{Union{Integer, AbstractUnitRange}}}) in StructArrays at /home/aplavin/.julia/packages/StructArrays/5C8nj/src/structarray.jl:304
Possible fix, define
  similar(::StructArray, ::Type{T}, ::Tuple{Union{Integer, Base.OneTo}, Vararg{Union{Integer, Base.OneTo}}}) where T

This PR fixes the ambiguity, and adds corresponding tests.

I'm not really familiar with the similar() machinery, but the signature in this PR follow the Base recommendation at https://github.com/JuliaLang/julia/blob/master/base/abstractarray.jl#L810-L814. Specifically, there should be Union{Integer, UnitRange} (this PR) instead of Union{Integer, AbstractUnitRange} (0.6.9).

@aplavin
Copy link
Member Author

aplavin commented Jun 19, 2022

@piever maybe the 0.6.9 release should even be yanked from General? In addition to map() and similar(), reshape() methods also got broken due to ambiguities. Caused by #218.

@piever
Copy link
Collaborator

piever commented Jun 19, 2022

Wow, that's pretty bad... Definitely surprising that something like this was not caught by tests. I agree we should yank the release (JuliaRegistries/General#62675) so that we can figure this out without having to rush.

I confess I am confused because OffsetArrays itself does not follow the guideline suggested by the Base comment (see here), but I suppose they can get away with it as they don't specialize on array type.

CC: @timholy (to double check that the approach in this PR is 100% correct)

Fixes: #234

@aplavin
Copy link
Member Author

aplavin commented Jun 20, 2022

My original commit in this PR definitely doesn't eliminate all ambiguitites. I ran Aqua.jl ambiguity detection, and it found a couple more (mostly in reshape) that are realizable. I wanted to add Aqua tests to the testsuite, but didn't: it has false-positives and reports ambiguities that cannot be realized due to the existince of more specific methods.

Also, the AbstractUnitRange is actually needed: similar should work with e.g. Base.IdentityUnitRange.

See the 2nd commit that fixes these issues.

src/structarray.jl Outdated Show resolved Hide resolved
src/structarray.jl Outdated Show resolved Hide resolved
src/structarray.jl Show resolved Hide resolved
src/structarray.jl Outdated Show resolved Hide resolved
@piever
Copy link
Collaborator

piever commented Jun 21, 2022

I've left a few minor comments, but this seems good. It is slightly worrying to me that the signatures become so complex, but I don't really see a way around it.

@aplavin
Copy link
Member Author

aplavin commented Jun 21, 2022

It is slightly worrying to me that the signatures become so complex, but I don't really see a way around it.

Yes, unfortunately, there is no clean way for structarrays to say "whatever the axes are, similar on structarray should forward to similar calls on components" and at the same time for offsetarrays to say "whatever the array type is, similar with a unitrange should create an offsetarray".
Some priority-based mechanism would be nice, so that structarrays dispatch is the first one. Not sure how feasible is that...

@piever piever merged commit b398788 into JuliaArrays:master Jun 23, 2022
@aplavin aplavin deleted the patch-1 branch June 23, 2022 12:03
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