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

Lazy broadcasting bug? #38

Open
mcabbott opened this issue May 30, 2019 · 4 comments
Open

Lazy broadcasting bug? #38

mcabbott opened this issue May 30, 2019 · 4 comments

Comments

@mcabbott
Copy link
Contributor

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
Copy link
Member

tkf commented May 30, 2019

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.

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

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
Copy link
Member

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

@mcabbott
Copy link
Contributor Author

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
Copy link
Member

tkf commented May 31, 2019

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 JuliaLang/julia#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.

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

No branches or pull requests

3 participants