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

Type assert error in broadcasting on nightly #38422

Closed
KristofferC opened this issue Nov 13, 2020 · 5 comments · Fixed by #38428 or #39185
Closed

Type assert error in broadcasting on nightly #38422

KristofferC opened this issue Nov 13, 2020 · 5 comments · Fixed by #38428 or #39185
Assignees
Milestone

Comments

@KristofferC
Copy link
Sponsor Member

Running the tests for AutomotiveSimulator errors on nightly (https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/c3bb6df_vs_788b2c7/AutomotiveSimulator.1.6.0-DEV-a896693d33.log) due to a type assert error in broadcasting.

feature extraction: Error During Test at /home/kc/PkgEval16/dev/AutomotiveSimulator/test/test_features.jl:119
  Got exception outside of a @test
  TypeError: in typeassert, expected AbstractVector{var"#s825"} where var"#s825"<:(AbstractVector{var"#s825"} where var"#s825"<:Union{Missing, Float64}), got a value of type Vector{Vector{T} where T}
  Stacktrace:
    [1] copy(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Tuple{Base.OneTo{Int64}}, AutomotiveSimulator.var"#25#26"{typeof(dist_to_front_neighbor), Roadway{Float64}, Vector{EntityScene{VehicleState, VehicleDef, Int64}}}, Tuple{Vector{Int64}}})
      @ Base.Broadcast ./broadcast.jl:928
    [2] materialize
      @ ./broadcast.jl:881 [inlined]
    [3] broadcast(f::AutomotiveSimulator.var"#25#26"{typeof(dist_to_front_neighbor), Roadway{Float64}, Vector{EntityScene{VehicleState, VehicleDef, Int64}}}, As::Vector{Int64})
      @ Base.Broadcast ./broadcast.jl:819
    [4] extract_feature(ft::SceneFeature, feature::Function, roadway::Roadway{Float64}, scenes::Vector{EntityScene{VehicleState, VehicleDef, Int64}}, ids::Vector{Int64})
      @ AutomotiveSimulator ~/PkgEval16/dev/AutomotiveSimulator/src/feature-extraction/features.jl:96
....

This type assert comes from:

julia/base/broadcast.jl

Lines 924 to 928 in 3dc54a2

# The typeassert gives inference a helping hand on the element type and dimensionality
# (work-around for #28382)
ElType′ = ElType <: Type ? Type : ElType
RT = dest isa AbstractArray ? AbstractArray{<:ElType′, ndims(dest)} : Any
return copyto_nonleaf!(dest, bc′, iter, state, 1)::RT

and was introduced in #30485.

Is the package doing something wrong or is this type assert a bit too strict?

cc @nalimilan

@nalimilan
Copy link
Member

Ouch. The code where this happens is a series of nested broadcast calls on vectors of vectors. After digging using Cthulhu and print statements, I found a very simple reproducer:

julia> x = Vector{<:Union{Int, Missing}}[[1, 2],[missing, 1]]
2-element Vector{Vector{var"#s1"} where var"#s1"<:Union{Missing, Int64}}:
 [1, 2]
 Union{Missing, Int64}[missing, 1]

julia> identity.(x)
ERROR: TypeError: in typeassert, expected AbstractVector{var"#s823"} where var"#s823"<:(Vector{var"#s1"} where var"#s1"<:Union{Missing, Int64}), got a value of type Vector{Vector{T} where T}
Stacktrace:
 [1] copy
   @ ./broadcast.jl:928 [inlined]
 [2] materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(identity), Tuple{Vector{Vector{var"#s1"} where var"#s1"<:Union{Missing, Int64}}}})
   @ Base.Broadcast ./broadcast.jl:881
 [3] top-level scope
   @ REPL[12]:1

This is due to this discrepancy:

julia> Base.Broadcast.promote_typejoin_union(Vector{<:Union{Int, Missing}})
Vector{var"#s1"} where var"#s1"<:Union{Missing, Int64} (alias for Array{var"#s1", 1} where var"#s1"<:Union{Missing, Int64})

julia> Base.promote_typejoin(Vector{Int}, Vector{Missing})
Vector{T} where T (alias for Array{T, 1} where T)

#38428 has a simple fix until we find a better solution.

@kalmarek
Copy link
Contributor

So I'm experiencing something very similar on 1.6.0-beta1:

using Pkg; Pkg.add("Cyclotomics")
using Cyclotomics
E(3).*[1,2,3]

produces

ERROR: TypeError: in typeassert, expected AbstractVector{var"#s827"} where var"#s827"<:Type, got a value of type Vector{Cyclotomic{Int64, SparseArrays.SparseVector{Int64, Int64}}}
Stacktrace:
 [1] copy
   @ ./broadcast.jl:930 [inlined]
 [2] materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(*), Tuple{Cyclotomic{Int64, SparseArrays.SparseVector{Int64, Int64}}, Vector{Int64}}})
   @ Base.Broadcast ./broadcast.jl:883
 [3] top-level scope
   @ REPL[3]:1

A Cyclotomic <: Number, is a weighted sum of roots of unity, hence it's a complex number.
On the other hand I implemented iteration which returns pairs (exponent, coefficient), i.e.
eltype(::Cyclotomic{T, ...}) = Tuple{Int, T}.

Note that wrapping with Ref solves the problem:

julia> Ref(E(3)) .* [1,2,3]
3-element Vector{Cyclotomic{Int64, SparseArrays.SparseVector{Int64, Int64}}}:
    ζ₃
  2*ζ₃
  3*ζ₃

This code worked just fine on julia-1.5 and before

@nalimilan
Copy link
Member

Argh, this change seems to trigger bugs in lots of corner cases... In this case, it seems to be because eltype(::Cyclotomic) returns Tuple{Int, T} while getindex returns a Vector. #39185 fixes the error, but I'm not sure it's a good idea to do things like that, as you're likely to confuse users and trigger weird bugs like this in many places.

@kalmarek
Copy link
Contributor

Argh, this change seems to trigger bugs in lots of corner cases... In this case, it seems to be because eltype(::Cyclotomic) returns Tuple{Int, T} while getindex returns a Vector. #39185 fixes the error, but I'm not sure it's a good idea to do things like that, as you're likely to confuse users and trigger weird bugs like this in many places.

Thanks, that's lightning fast!
I see; I'll try to amend this discrepancies, it should be easy to fix on my side;

I know this eltype(x) == typeof(first(x)) is not formal requirement, but maybe this advice could be added to docs somewhere?

@nalimilan
Copy link
Member

Thanks, that's lightning fast!
I see; I'll try to amend this discrepancies, it should be easy to fix on my side;

OK, good.

I know this eltype(x) == typeof(first(x)) is not formal requirement, but maybe this advice could be added to docs somewhere?

Yes, that sounds like a good idea. The current docstring for eltype mentions that it's supposed to correspond to the type of elements returned by iteration (which includes first), but it doesn't say anything about getindex (which is the problem in your case). Would you make a PR or file an issue about this?

kalmarek added a commit to kalmarek/julia that referenced this issue Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants