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

In-place broadcasting into non-broadcastable things #36105

Closed
mbauman opened this issue Jun 1, 2020 · 7 comments
Closed

In-place broadcasting into non-broadcastable things #36105

mbauman opened this issue Jun 1, 2020 · 7 comments
Labels
broadcast Applying a function over a collection needs decision A decision on this change is needed
Milestone

Comments

@mbauman
Copy link
Member

mbauman commented Jun 1, 2020

So we have the syntax A .= x, broadcasting x into A's shape and mutating A to those new values. I've always thought of this syntax only supporting broadcastable LHSes, but prior to #35620 this wasn't actually necessary... the default broadcast machinery seamlessly transformed this into a call to copyto!(A, x) without requiring much of A at all.

Version 1.4

julia> struct Foo end
       Base.size(::Foo) = ()
       Base.copyto!(::Foo, x) = "custom .= implementation"

julia> Foo() .= 1
"custom .= implementation"

Version 1.5-beta.1

julia> struct Foo end
       Base.size(::Foo) = ()
       Base.copyto!(::Foo, x) = "custom .= implementation"

julia> Foo() .= 1
ERROR: MethodError: no method matching ndims(::Type{Foo})

julia> Base.ndims(::Type{Foo}) = 1

julia> Foo() .= 1
"custom .= implementation"

This snagged several packages:

  • HomotopyContinuation defines a WeightedNorm that wraps a vector but isn't itself a vector or even iterable. It does, however, support copyto! and size, and its tests use .= to simply change the wrapped vector. It is currently not possible to use a WeightedNorm in any other broadcast expression.
  • DataFrames also got snagged by this, but they already fixed it and tagged a new version. Two other packages haven't updated to version 0.21 yet and thus hit this themselves — but will be fixed upon upgrading to the new version (Diversity.jl and RevealedPreferences.

So the question is: are we ok with this change? I'm inclined to say that the existing usages are incomplete specifications.

@mbauman mbauman added broadcast Applying a function over a collection needs decision A decision on this change is needed triage This should be discussed on a triage call labels Jun 1, 2020
@mbauman mbauman added this to the 1.5 milestone Jun 1, 2020
@mbauman
Copy link
Member Author

mbauman commented Jun 16, 2020

Both packages snagged by this agreed they had an incomplete specification and have already fixed it themselves. I think the gain in functionality is worth this very minor change, and I would advocate for no additional changes here.

@tkf
Copy link
Member

tkf commented Jun 16, 2020

I suppose 1.4-like behavior is still possible by overloading materialize! and taking over the style promotion? If so, I guess nothing is essentially lost?

(Of course, opening up materialize! as a public API or not is another question.)

@mbauman
Copy link
Member Author

mbauman commented Jun 16, 2020

Yes, that would work, but the better way (and the way both those packages pursued) is simply to "finish" implementing the broadcastable interface or avoid this idiom entirely.

@tkf
Copy link
Member

tkf commented Jun 16, 2020

Yeah, I agree that's the right move, e.g., for the packages you listed in the OP.

I was thinking about extending the semantics of broadcasting where axes makes less sense. Something like nested arrays and dictionaries (like Python's awkward-array; Related discussion with @andyferris in andyferris/Dictionaries.jl#19).

@andyferris
Copy link
Member

andyferris commented Jun 16, 2020

I agree that the awkward-array style operations are good target.

For nested things @tkf I wonder if we could using nested dot-getindices operations to detangle it on the LHS, as it seems (to me) you might need to do anyway on the RHS?

A.[2:4].[end] .= B.[1:3].[begin]

(I'm not sure if that particular example makes sense, but something like that)

@tkf
Copy link
Member

tkf commented Jun 16, 2020

(Just a quick response, but maybe #30845 is the best place to continue the discussion?)

@mbauman
Copy link
Member Author

mbauman commented Jun 16, 2020

Yes, and I think this issue can be closed.

@mbauman mbauman closed this as completed Jun 16, 2020
@mbauman mbauman removed the triage This should be discussed on a triage call label Jun 16, 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 needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

3 participants