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

Recommend to use tuples to scalarize items in broadcast expressions #35591

Merged
merged 7 commits into from
Apr 29, 2020

Conversation

MasonProtter
Copy link
Contributor

@MasonProtter MasonProtter commented Apr 24, 2020

There's a lot of conflicting advice out there on what one should do in order to treat a given object as a scalar in a broadcast expression. Perhaps the most common practice is to use Ref, but this is arguably not the intended use of Ref and can be less performant than tuple because Ref (currently) blocks constant propagation. For a contrived example:

julia> using StaticArrays, BenchmarkTools

julia> us = (SA[1,2,3], SA[4,5,6]); v = SA[7,8,9]
3-element SArray{Tuple{3},Int64,1,3} with indices SOneTo(3):
 7
 8
 9

julia> @btime $us .+ Ref($v)
  2.839 ns (0 allocations: 0 bytes)
([8, 10, 12], [11, 13, 15])

julia> @btime $us .+ ($v,)
  0.020 ns (0 allocations: 0 bytes)
([8, 10, 12], [11, 13, 15])

This difference is perhaps inconsequential currently, but in a future where Julia is better able to reason about non-mutated arrays it could become more important, so I think it's better to recommend a more future-proof style. Of course, one could say that in that future Ref might also play better with constant prop, but I think that may be undesirable because Ref is often used for explicitly blocking constant prop, i.e. in benchmarking.


I also modified an example that wrote ceil.((UInt8,), [1.2 3.4; 5.6 6.7]) to instead say ceil.(UInt8, [1.2 3.4; 5.6 6.7]) because there is no reason to wrap UInt8 in a tuple since it is already a broadcast scalar.

@tkf
Copy link
Member

tkf commented Apr 24, 2020

Ah, it's good to know that there is a pitfall with Ref.

But I think the problem with recommending using tuples is that a tuple is not always the "weakest container":

julia> 1 .+ (2,)
(3,)

julia> 1 .+ Ref(2)
3

Perhaps we need an immutable version of Ref (i.e., stack-allocatable zero-dimensional container)?

@mkborregaard
Copy link
Contributor

mkborregaard commented Apr 24, 2020

It might be unrelated to the issue at hand to discuss surface syntax, but I'd think

us .+ &v

was even clearer

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Apr 24, 2020

@tkf Great point, I hadn't realized that. Such a stack allocated 0-dimensional container is indeed fairly easy to make. Here's one that's similar (but not the same as StaticArrays.Scalar which doesn't have the behaviour you desire behaviour here):

struct Scalar{T}
    x::T
end
Base.getindex(s::Scalar) = s.x
Base.getindex(s::Scalar, ::CartesianIndex{0}) = s.x
Base.iterate(s::Scalar) = (s.x, nothing)
Base.iterate( ::Scalar, s) = nothing
Base.ndims(::Scalar) = 0
Base.ndims(::Type{<:Scalar}) = 0
Base.length(::Scalar) = 1
Base.eltype(::Scalar{T}) where {T} = T
Base.eltype(::Type{Scalar{T}}) where {T} = T
Base.size(::Scalar) = ()
Base.axes(::Scalar) = ()
Base.IteratorSize(::Type{<:Scalar}) = Base.HasShape{0}()
Base.broadcastable(s::Scalar) = s

now

julia> using StaticArrays: SA

julia> using BenchmarkTools: @btime

julia> us = (SA[1,2,3], SA[4,5,6]); v = SA[7,8,9]
3-element StaticArrays.SArray{Tuple{3},Int64,1,3} with indices SOneTo(3):
 7
 8
 9

julia> @btime $us .+ Ref($v)
  2.849 ns (0 allocations: 0 bytes)
([8, 10, 12], [11, 13, 15])

julia> @btime $us .+ Scalar($v)
  0.019 ns (0 allocations: 0 bytes)
([8, 10, 12], [11, 13, 15])

julia> @btime 1 .+ Scalar(1)
  0.019 ns (0 allocations: 0 bytes)
2

@mkborregaard Yeah, that syntax would be nice for this, but my understanding is that syntax proposal is controversial so I'd rather not stir up the hornets nest in this PR which is of relatively limited scope.

@tkf
Copy link
Member

tkf commented Apr 25, 2020

but I think that may be undesirable because Ref is often used for explicitly blocking constant prop, i.e. in benchmarking.

I think clobber/escape JuliaCI/BenchmarkTools.jl#92 would be the best strategy for this. Having a specific tool to do this rather than relying on an inability of the compiler sounds like a good approach to me. I think it'd be great if constant propagation can penetrate mutable objects when they are elided.

@tkf
Copy link
Member

tkf commented Apr 25, 2020

It's great that you get Scalar working! FYI I found an old (but still open) discussion here: #18379

@ararslan ararslan requested a review from mbauman April 26, 2020 21:39
@ararslan ararslan added broadcast Applying a function over a collection docs This change adds or pertains to documentation labels Apr 26, 2020
@mbauman
Copy link
Member

mbauman commented Apr 27, 2020

The only reason I've not done this sooner myself is that a one-tuple doesn't really act like a scalar. For example:

Refs and other 0-dimensional containers actually are treated exactly like scalars. 1-tuples are not. In practice it's generally fine, but I don't really like calling this the "best way to treat something like a scalar."

@MasonProtter
Copy link
Contributor Author

Yeah, that's fair I hadn't realized that subtlety until @tkf pointed it out. What do you think about that Scalar type (maybe with a different name)? Would it be just extra cruft if we added it to base?

@mbauman
Copy link
Member

mbauman commented Apr 27, 2020

I actually initially used such a type internally in initial sketches until I realized we could use Ref and there was the promise of that & syntax. Further, I wasn't able to see any performance hit to using a mutable container as everything inlines... but this predated constant propagation!

@MasonProtter
Copy link
Contributor Author

Do you think the fact that we have constant propagation now warrants revisiting this design?

@mbauman
Copy link
Member

mbauman commented Apr 27, 2020

Yes, I would love for a better pattern here. It's a common stumbling block, and to me that's a far bigger motivation for this change than the performance issue.

I have a fairly strong aversion to the name Scalar, though, because it's explicitly not a scalar. It's a zero-dimensional container. Yes, it means that what it contains gets treated as though it were a scalar, but that's because this Scalar thing itself is a container!

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Apr 27, 2020

Sounds good. Do you or anyone else have name suggestions? Wrap{T}?

@tkf
Copy link
Member

tkf commented Apr 27, 2020

I guess Singleton makes sense? It's a bit long and an overloaded word, though.

@ViralBShah
Copy link
Member

Bump. What's needed to merge this?

@MasonProtter
Copy link
Contributor Author

@ViralBShah The potentially blocking concern currently is that Tuples don't really behave like scalars. Unlike Ref, (1,) .+ 1 == (2,) != 2.

If it's okay with people, I could ammend the wording to instead say something like

if you don't want to broadcast into a given container, you can wrap it in a single argument Tuple

and then give the same examples, that way I don't have misleading terms like "scalar".

@MasonProtter
Copy link
Contributor Author

Done.

base/refpointer.jl Show resolved Hide resolved
doc/src/manual/arrays.md Outdated Show resolved Hide resolved
@tkf
Copy link
Member

tkf commented Apr 28, 2020

It's somewhat off-topic, but I think it is somewhat inconsistent that

julia> (20,) .+ ones(2, 2)
2×2 Array{Float64,2}:
 21.0  21.0
 21.0  21.0

actually works even though

julia> (20,)[1, 1]
ERROR: MethodError: no method matching getindex(::Tuple{Int64}, ::Int64, ::Int64)
...

because we often mention that [20] .+ ones(2, 2) works because [20][1, 1] works.

I guess it's another reason to have "immutable Ref" somewhere.

@mbauman
Copy link
Member

mbauman commented Apr 28, 2020

because we often mention that [20] .+ ones(2, 2) works because [20][1, 1] works.

Funny, that's not my mental model at all. I think of tuples (and other non-array containers) as being one dimensional, even if they don't support n-dimensional access, and broadcast just extrudes over both missing and singleton dimensions.

In other words, in my mental model, the array behavior is the special case (that it allows indexing by missing dimensions)... and it's not what drives broadcasting behavior.

@tkf
Copy link
Member

tkf commented Apr 28, 2020

Ah, OK. I thought that's what you would say as (I think) I've seen you were mentioning that numbers being iterable and indexable is nice because it's compatible with broadcasting. But I agree that filling extra dimensions fits well with what we have.

@MasonProtter
Copy link
Contributor Author

I believe this PR is now good to go.

@mbauman mbauman merged commit 99faf1c into JuliaLang:master Apr 29, 2020
@MasonProtter MasonProtter deleted the patch-1 branch April 29, 2020 23:51
@rfourquet
Copy link
Member

On bikesheding the name: instead of Scalar, what about Only? only is already used in a container context with only one element, and there would be the property only(Only(x)) === x.

@tkf
Copy link
Member

tkf commented May 6, 2020

@rfourquet The discussion is happening at #35675.

@rfourquet
Copy link
Member

Ah thanks for the pointer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants