JuliaArrays / LazyArrays.jl

Lazy arrays and linear algebra in Julia
MIT License
306 stars 28 forks source link

Lazy broadcasting bug? #38

Open mcabbott opened 5 years ago

mcabbott commented 5 years ago

I just ran into the following error:

julia> R = rand(3); s = 1;

julia> using LazyArrays: lazy

julia> @. lazy(2 * s * R)
3-element LazyArrays.BroadcastArray{Float64,1,Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(*),Tuple{Int64,Int64,Array{Float64,1}}}}:
 0.9510075392608135 
 1.3657778460965129 
 0.34610423904646703

julia> @. lazy((2s) * R)
ERROR: MethodError: no method matching LazyArrays.BroadcastArray{Int64,N,BRD} where BRD<:Base.Broadcast.Broadcasted where N(::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(*),Tuple{Int64,Int64}})

The same happens for @~ @. (2s) * R obviously.

This is for LazyArrays v0.8.1, Julia 1.1. On master, #31 has changed things such that I get the error only when calling LazyArray:

julia> @. lazy(2 * s * R)
Base.Broadcast.Broadcasted(*, (2, 1, [0.147668, 0.659113, 0.367685]))

julia> @. lazy((2s) * R)
Base.Broadcast.Broadcasted(*, (Base.Broadcast.Broadcasted(*, (2, 1)), [0.147668, 0.659113, 0.367685]))

julia> LazyArray(@. lazy(2 * s * R))
3-element BroadcastArray{Float64,1,Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(*),Tuple{Int64,Int64,Array{Float64,1}}}}:
 0.29533639092844455
 1.318225163324581  
 0.7353707033373191 

julia> LazyArray(@. lazy((2s) * R))
ERROR: MethodError: no method matching BroadcastArray{Int64,N,BRD} where BRD<:Base.Broadcast.Broadcasted where N(::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(*),Tuple{Int64,Int64}})
tkf commented 5 years ago

This is because 2s is a scalar and LazyArrays.jl does not have a lazy representation for it at the moment. More minimal example would be:

julia> LazyArray(@~ 1 .* 2)
ERROR: MethodError: no method matching BroadcastArray{Int64,N,BRD} where BRD<:Broadcasted where N(::Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(*),Tuple{Int64,Int64}})
Closest candidates are:
  BroadcastArray{Int64,N,BRD} where BRD<:Broadcasted where N(::Broadcasted{#s16,#s15,#s14,#s13} where #s13<:Tuple where #s14 where #s15<:Tuple{Vararg{Any,N}} where #s16<:Union{Nothing, BroadcastStyle}) where {T, N} at /home/takafumi/.julia/dev/LazyArrays/src/lazybroadcasting.jl:13
Stacktrace:
 [1] _BroadcastArray(::Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(*),Tuple{Int64,Int64}}) at /home/takafumi/.julia/dev/LazyArrays/src/lazybroadcasting.jl:19
 [2] BroadcastArray(::Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(*),Tuple{Int64,Int64}}) at /home/takafumi/.julia/dev/LazyArrays/src/lazybroadcasting.jl:20
 [3] LazyArray(::Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(*),Tuple{Int64,Int64}}) at /home/takafumi/.julia/dev/LazyArrays/src/lazybroadcasting.jl:10
 [4] top-level scope at none:0

julia> typeof(@~ 1 .* 2)
Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(*),Tuple{Int64,Int64}}

BroadcastArray needs the second type parameter Axes to be a Tuple but it's Nothing for scalar broadcasting above.

https://github.com/JuliaArrays/LazyArrays.jl/blob/3b6be0667fa843d9200eb526e756f7fd93b61661/src/lazybroadcasting.jl#L13-L15

Adding the following definition fixes this issue:

BroadcastArray{T}(bc::Broadcasted{<:Union{Nothing,BroadcastStyle},Nothing,<:Any,<:Tuple}) where {T} =
    BroadcastArray{T,0}(bc)

But I wonder why Base.Broadcast chose to use Nothing for Axes instead of Tuple{}. If there are a good reason for why distinguishing Nothing and Tuple{}, it may not be safe to add above definition.

dlfivefifty commented 5 years ago

I guess what one would one from a lazy scalar is to somehow preserve constant propagation?

mcabbott commented 5 years ago

Thanks, that's much simpler than my example!

The suggested change means that LazyArray(@~ 1 .* 2) is a 0-array, and this is preserved by broadcasting with scalars. But ordinary broadcasting of say (Array{Int,0}(undef) .= 3) with a scalar results in a scalar -- might that be what the Nothing is signalling?

It's possible that you should instead just not be lazy about scalar + scalar broadcasting at all. Are there examples where scalar laziness would be desirable? It would seem strange for the lazy version to return a container when the ordinary one does not.

tkf commented 5 years ago

FYI: I just dug into the code a bit more. The special handling does seem to be intentional. It's explicitly defined that

instantiate(bc::Broadcasted{<:Union{AbstractArrayStyle{0}, Style{Tuple}}}) = bc

in https://github.com/JuliaLang/julia/blob/48634f9f8669e1dc1be0a1589cd5be880c04055a/base/broadcast.jl#L263 which committed in https://github.com/JuliaLang/julia/pull/26891; i.e., it was there from the very beginning. But I'm not sure what it means...

@dlfivefifty I don't quite get what you are saying. But I guess you mean it was some kind of performance optimization? So it's safe to handle scalars as 0-dim array?

@mcabbott

Are there examples where scalar laziness would be desirable?

It's a bit contrived example, but @~ Ref(A) .* Ref(B) is an lazy operations of "scalars" but A and B could actually be large matrices.

It would seem strange for the lazy version to return a container when the ordinary one does not.

copy/materizlie works as the normal broadcasting:

julia> copy(@~ 1 .* 2) :: Int
2

julia> Broadcast.materialize(@~ 1 .* 2) :: Int
2

I think it's OK for LazyArray(@~ 1 .* 2) to return a 0-dim array because you explicitly asks for an array this way. I think you'd expect T(...) isa T to hold usually.