-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Simplify broadcastable a bit further and add docs #26601
Conversation
Amazing what a great exercise writing documentation is. This is a followup to #26435, and more clearly lays out the requirements for defining a custom (non-AbstractArray) type that can participate in broadcasting. The biggest functional change here is that types that define their own BroadcastStyle must now also implement `broadcastable` to be a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failure is
LoadError("sysimg.jl", 273, LoadError("broadcast.jl", 11, LoadError("broadcast.jl", 11, UndefVarError(Symbol("@__dot__")))))
doc/src/manual/interfaces.md
Outdated
derived. When used as a function it has two possible forms, | ||
unary (single-argument) and binary. | ||
The unary variant states that you intend to | ||
implement specific broadcasting behavior and/or output type, | ||
and do not wish to rely on the default fallback ([`Broadcast.Scalar`](@ref) or [`Broadcast.DefaultArrayStyle`](@ref)). | ||
To achieve this, you can define a custom `BroadcastStyle` for your object: | ||
and do not wish to rely on the default fallback ([`Broadcast.DefaultArrayStyle`](@ref))). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One too many )
s
doc/src/manual/interfaces.md
Outdated
@@ -444,6 +444,7 @@ V = view(A, [1,2,4], :) # is not strided, as the spacing between rows is not f | |||
| **Optional methods** | | | | |||
| `Base.BroadcastStyle(::Style1, ::Style2) = Style12()` | Precedence rules for mixing styles | | |||
| `Base.broadcast_indices(::StyleA, A)` | Declaration of the indices of `A` for broadcasting purposes (for AbstractArrays, defaults to `axes(A)`) | | |||
| `Base.broadcastable(x)` | Convert `x` to an object that has [`axes`] and supports indexing | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[`axes`](@ref)
or just `axes`
doc/src/manual/interfaces.md
Outdated
types of its arguments and collapse them down to just one output array and one | ||
implementation. Broadcast calls this single answer a "style." Every broadcastable object | ||
each has its own preferred style, and a promotion-like system is used to combine these | ||
styles into a single answer — the "desination style". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
desination
-> destination
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! (On all counts)
It seems like the
What is wrong with something like broadcastable(x) = _broadcastable(x, BroadcastStyle(typeof(x))
_broadcastable(x, ::Unknown) = ...deprecation...
_broadcastable(x, s) = x so that no-op is the default for types that define their own |
This started as a documentation issue and morphed into the code changes that were required to make the documentation simple and understandable. Yes, I agree the need for the type to support As far as your other suggestion, the issue with doing it that way is that then every type that wants to participate in broadcasting must both define IMO: |
Broadcasting a In the type-stable case, you don't lose anything by making |
It's a fairly disruptive change since everyone currently defines their
But that requires an optimized and inferred |
It’s disruptive for people using an unreleased 0.7 feature... isn’t this precisely the time to fix the API? |
Besides, it could be backwards compatible if the instance-based constructor were defined as lowercase broadcaststyle(x). This would be more consistent with most of our traits functions anyway. |
Yeah, we're in agreement here. Are you saying you want me to do that in this PR? I'd still prefer to do it separately. |
Separately is fine with me. |
Closing and re-opening to re-run CI — I'll plan to merge this tomorrow and then followup with the proposed changes to |
Amazing what a great exercise writing documentation is. This is a followup to #26435, and more clearly lays out the requirements for defining a custom (non-AbstractArray) type that can participate in broadcasting. The biggest functional change here is that types that define their own BroadcastStyle must now also implement
broadcastable
to be a no-op.