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

Support broadcast assignment to Ref objects #34228

Closed
wants to merge 3 commits into from

Conversation

jmert
Copy link
Contributor

@jmert jmert commented Dec 31, 2019

This is a pretty straight-forward extension of adding CartesianIndex() access in #32653 — I was motivated by trying to develop code which exploits broadcasting to operate on anything from scalars to multidimensional arrays using CartesianIndices indexing. This doesn't work for a Ref-wrapped scalar without these definitions, though.

@mbauman
Copy link
Member

mbauman commented Dec 31, 2019

If the analogy is that Refs are like 0-dimensional arrays, then shouldn't this just return a Ref right back at you?

julia> A = fill(1)
0-dimensional Array{Int64,0}:
1

julia> A[CartesianIndices(())]
0-dimensional Array{Int64,0}:
1

Or perhaps more to the point, if we support this, I think we should support all AbstractArray{CartesianIndex{0},0}s. But I'd be very hesitant to do either.

@jmert
Copy link
Contributor Author

jmert commented Dec 31, 2019

Yes, it seems you're right — dropping the Ref wrapper isn't what I want. With getindex unwrapping, I hit a new error in broadcasting, and it's the same as if the code assumes the left hand side is a Ref:

julia> g1!(x) = x[] .= 1.0
g1! (generic function with 1 method)

julia> g1!(Ref(1.0))
ERROR: MethodError: no method matching copyto!(::Float64, ::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Tuple{},typeof(identity),Tuple{Float64}})
...

Maybe I should poke at the broadcasting behavior instead?

@jmert
Copy link
Contributor Author

jmert commented Jan 1, 2020

It looks like if I instead define:

@eval Base begin
    maybeview(A::Ref, ::CartesianIndex{0}) = A
    maybeview(A::Ref, ::CartesianIndices{0,Tuple{}}) = A
end
@eval Base.Broadcast begin
    @inline function copyto!(dest::Ref, bc::Broadcasted{<:AbstractArrayStyle{0}})
        dest[] = materialize(bc)
    end
end

I can get the behavior I wanted with something like:

julia> g1!(x) = (x .= 1.0; x)
g1! (generic function with 1 method)

julia> g1!(Ref(0.0))
Base.RefValue{Float64}(1.0)

Is this a better idea? I suppose the alternative is to replicate a Ref-like struct and add similar definitions to it instead.

@jmert jmert force-pushed the ref_cartesianindices branch from 1485c27 to 3e430c6 Compare January 2, 2020 01:31
@jmert jmert changed the title Add CartesianIndices(()) for Ref getindex and setindex! Support broadcast assignment to Ref objects Jan 2, 2020
@fredrikekre fredrikekre added the broadcast Applying a function over a collection label Jan 10, 2020
@jmert jmert closed this Jul 5, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants