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

RFC: Drop BroadcastStyle back down to the value domain #26941

Closed
wants to merge 1 commit into from

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented May 1, 2018

This is a subtle change for broadcast implementors -- instead of defining BroadcastStyle(::Type{<:MyType}), they now need to define BroadcastStyle(::MyType) directly. This is a breaking change to a new API in 0.7; so it's not technically breaking over 0.6, but any libraries that have adapted to the new API will need to shift their definitions. The primary purpose of this change is becasue the BroadcastStyle needs to know about the ndims of its passed argument, but some arguments (like PyArrays) don't encode ndims in the type domain. Asking the value directly allows them to participate more fully in the broadcast interface.

Ref #26601 (comment).

@mbauman mbauman added the broadcast Applying a function over a collection label May 1, 2018
@nalimilan
Copy link
Member

Maybe provide a BroadcastStyle(x) = BroadcastStyle(typeof(x)) fallback? That's what we usually do for traits (e.g. IteratorSize), and that would avoid breaking packages which have already been ported to the 0.7 API.

@mbauman
Copy link
Member Author

mbauman commented May 2, 2018

That gives me pause since we support types themselves in broadcast, leading to a potential ambiguity in meaning when doing, e.g., f.(Tuple, 1:2). That said, we're saved by broadcastable, which wraps types in a Ref, so perhaps this is indeed a good idea, but it still feels a little fragile.

@stevengj
Copy link
Member

stevengj commented May 2, 2018

That fallback would presumably be in a @deprecate method, so if it is a little fragile that is not too bad, being temporary.

@nalimilan
Copy link
Member

It should be fine as a deprecated method, but otherwise I agree the ambiguity would be annoying.

However this raises a broader issue regarding other traits. For example, IteratorSize is supposed to work when called on a type, but that's not possible for PyArrays since HasShape{N} needs to know the dimension. Yet, it's essential to be able to check whether a type implements a trait without having an instance of it...

@mbauman
Copy link
Member Author

mbauman commented May 2, 2018

This is complicated to deprecate because we already have an ::Any fallback, returning a DefaultArrayStyle. I went through a few contortions to get it working, but even then it didn't meaningfully work:

julia> using StaticArrays

julia> v = @SArray [1,2,3]
3-element SArray{Tuple{3},Int64,1,3}:
 1
 2
 3

julia> v .* 3
┌ Warning: the BroadcastStyle API is changing. The implementation of StaticArrays.StaticArrayStyle{1}() should work on values, not types.
│   caller = Type at broadcast.jl:100 [inlined]
└ @ Core broadcast.jl:100
3-element Array{Int64,1}:
 3
 6
 9

The broadcast API changed, so while we got the BroadcastStyle correct there, v .* 3 ends up calling copy(::Broadcasted{StaticArrays.StaticArrayStyle{1}}) and falling back to the default implementation anyways (since StaticArrays hasn't updated to specialize that method yet).

I don't think a fragile deprecation mechanism here is worth the trouble.

@mbauman mbauman force-pushed the mb/value-based-BroadcastStyle branch 2 times, most recently from 1a9d9f6 to 7fd3a9d Compare May 7, 2018 21:38
@mbauman
Copy link
Member Author

mbauman commented May 9, 2018

Yet, it's essential to be able to check whether a type implements a trait without having an instance of it

I'm trying to figure out just how important this is. I do agree that, in general, we want to do these sorts of computations in the type domain. In this case, though, I'm having a hard time imagining what sort of situation you'd want to pre-compute a BroadcastStyle based upon a given set of argument types without the arguments themselves. Indeed, the main API here has been combine_styles(args…) — which has and still does take values and not types.

You may want to explicitly call a given BroadcastStyle — and in that case you can just construct it directly yourself.

I suppose one might want to "swap out" a given argument and do the computation as though it were a different type. That'd be a situation where you'd want a type-based mechanism.

I'm still in favor of this PR.

@mbauman mbauman force-pushed the mb/value-based-BroadcastStyle branch from 7fd3a9d to 567b27c Compare May 14, 2018 21:13
This is a subtle change for broadcast implementors -- instead of defining `BroadcastStyle(::Type{<:MyType})`, they now need to define `BroadcastStyle(::MyType)` directly. This is a breaking change to a new API in 0.7; so it's not technically breaking over 0.6, but any libraries that have adapted to the new API will need to shift their definitions. The primary purpose of this change is becasue the `BroadcastStyle` needs to know about the `ndims` of its passed argument, but some arguments (like PyArrays) don't encode ndims in the type domain. Asking the value directly allows them to participate more fully in the broadcast interface.
@mbauman mbauman force-pushed the mb/value-based-BroadcastStyle branch from 567b27c to 7b5d9b4 Compare May 18, 2018 15:10
@mbauman
Copy link
Member Author

mbauman commented May 18, 2018

Linux failures were "Process timed out possibly waiting for a response. Process output found:\n"""\nPrivate key location for 'git@github.com':..."

32 bit appveyor timed out.

@JeffBezanson JeffBezanson added deprecation This change introduces or involves a deprecation triage This should be discussed on a triage call labels May 20, 2018
@JeffBezanson
Copy link
Member

I generally favor using the value domain instead of types, but our other traits operate on types, so I hesitate to do this. Instead, for example, PyArray could have a wrapper type that moves the number of dimensions to a type parameter. It seems like that would be needed anyway e.g. for all the dispatch we do on matrix vs. vector.

@StefanKarpinski
Copy link
Member

Triage concludes that any array type that doesn't have the dimensionality in the type is going to be pretty broken in Julia lang; other languages may change dimensionality at runtime but Julia doesn't and is at some fairly deep level against doing so. So the solution here seems to be always capturing the number of dimensions in the type.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label May 24, 2018
@DilumAluthge DilumAluthge deleted the mb/value-based-BroadcastStyle branch March 25, 2021 21:59
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 deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants