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

Simplify broadcastable a bit further and add docs #26601

Merged
merged 3 commits into from
Apr 4, 2018
Merged

Conversation

mbauman
Copy link
Sponsor Member

@mbauman mbauman commented Mar 23, 2018

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.

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

@fredrikekre fredrikekre left a 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__")))))

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One too many )s

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

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`

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

desination -> destination

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! (On all counts)

@stevengj
Copy link
Member

stevengj commented Mar 24, 2018

It seems like the BroadcastStyle(x) should be based on the value x, not on a type BroadcastStyle(T). (Imagine something like PyObject where the behavior might depend on the runtime value.) Is there a technical reason why it must take a type argument?

The biggest functional change here is that types that define their own BroadcastStyle must now also implement broadcastable to be a no-op.

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 BroadcastStyle?

@mbauman
Copy link
Sponsor Member Author

mbauman commented Mar 24, 2018

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 ndims is rather limiting. I'd like to address that in the future — it'd be nice to just use length(axes(arg)). Note, though, that it is required to be type-stable for a type-stable broadcast.

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 broadcastable (to avoid the collect fallback) and define their preferred BroadcastStyle (to avoid being classified as Unknown). How I was using Unknown before was kind-of an abuse — there isn't a base implementation for broadcasting Unknown styles. Indeed, the barebones Foo26601 would fail in surprising ways with that implementation. It's far more truthful to state that all types that quack like an array are DefaultArrayStyle. We just don't know if they'll quack like an array until after they become broadcastable.

IMO: broadcastable is the "simpler" API. It's the gateway into overriding behaviors. BroadcastStyle is far more complicated. It makes more sense to make the more complicated API require coordination with the simpler one than the other way around.

@stevengj
Copy link
Member

Note, though, that it is required to be type-stable for a type-stable broadcast.

Broadcasting a PyObject will basically never be type-stable, and that's okay.

In the type-stable case, you don't lose anything by making BroadcastStyle(x) take the object x rather than the type T, so you only gain flexibility this way.

@mbauman
Copy link
Sponsor Member Author

mbauman commented Mar 26, 2018

It's a fairly disruptive change since everyone currently defines their BroadcastStyle methods on the type. Note that we could get around this with a definition like:

BroadcastStyle(x::T) where {T} = DefaultArrayStyle{applicable(ndims, T) ? ndims(T) : Any}()

But that requires an optimized and inferred applicable. That's why I'd like to tackle it on its own.

@stevengj
Copy link
Member

It’s disruptive for people using an unreleased 0.7 feature... isn’t this precisely the time to fix the API?

@stevengj
Copy link
Member

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.

@mbauman
Copy link
Sponsor Member Author

mbauman commented Mar 26, 2018

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.

@stevengj
Copy link
Member

Separately is fine with me.

@mbauman
Copy link
Sponsor Member Author

mbauman commented Apr 3, 2018

Closing and re-opening to re-run CI — I'll plan to merge this tomorrow and then followup with the proposed changes to BroadcastStyle.

@mbauman mbauman closed this Apr 3, 2018
@mbauman mbauman reopened this Apr 3, 2018
@mbauman mbauman merged commit 416305c into master Apr 4, 2018
@mbauman mbauman deleted the mb/broadcastabledocs branch April 4, 2018 17:47
@mbauman mbauman added the domain:broadcast Applying a function over a collection label Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:broadcast Applying a function over a collection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants