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

issue-pr: extend sparse broadcast[!] to one- and two-dimensional Arrays #20007

Closed
wants to merge 1 commit into from

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jan 13, 2017

This pull request extends sparse broadcast[!] to one- and two-dimensional Arrays (via promotion to sparse, as with structured matrices), addressing #11474:

julia> sp = sprand(3,3,0.2)
3×3 sparse matrix with 2 Float64 stored entries:
  [2, 1]  =  0.475801
  [2, 3]  =  0.851515

julia> broadcast(*, [1,1,1], sp)
3×3 sparse matrix with 2 Float64 stored entries:
  [2, 1]  =  0.475801
  [2, 3]  =  0.851515

Though it works, this pull request probably should not be merged (or only as a stopgap). It serves mainly to illustrate an issue with the broadcast containertype promotion mechanism. The comments in this pull request describe that issue at length (partly copied below) followed by an illustration. I plan to sketch an implementation of the proposal in those comments in another PR. Best!

# Issue: `Array` has multiple meanings in the broadcast containertype promotion mechanism:
# (1) `Array`, in the sense of "this object is an Array (or this is a collection of Arrays)";
# (2) `AbstractArray`, in the sense of "this object is an AbstractArray (or this is a collection
#   of AbstractArrays) but where that AbstractArray is (/a subset of that collection of
#   AbstractArrays are) not of a specific AbstractArray subtype for which special
#   handling is defined;
# (3) `AbstractArray`, in the sense of "this is a collection of objects that need be funneled
#   to the generic AbstractArray broadcast code".
# Presently we conflate these three meanings in `Array`. Conflating meanings (2) and (3)
# might be fine, but conflating meaning (1) with the other two prevents separating objects
# that are Arrays (from e.g. collections of Arrays and Tuples) for special handling, as is
# necessary e.g. to handle Arrays in sparse broadcast.
#
# We should probably disambiguate these meanings in Broadcast. One approach to doing so
# would be to replace `Array` with `AbstractArray` in the existing containertype promotion
# mechanism, and then separately define containertype promotion methods for Array as we do
# for Tuple.
#
# The tricky question is what to do about the promote_containertype(ct, ::Type{Array}) = Array
# (after the change suggested above, promote_containertype(ct, ::Type{AbstractArray})) = AbstractArray)
# methods. These methods are ambiguity magnets, and there is a tendency to write similar
# such methods whenever extending broadcast to a new type, which ultimately results
# in having to write a combinatorial explosion of ambiguity-killing methods.
#
# Perhaps the following makes a reasonable model: don't define methods like
# promote_containertype(ct, ::Type{Array}) = Array, and discourage definition of such
# methods for new types. Instead (1) define promote_containertype(cta, ctb) = AbstractArray as
# primary fallback, such that any type pair without clearly defined behavior gets funneled
# to the generic AbstractArray broadcast code, and (2) encourage writing only explicit, tight
# promote_containertype definitions.
#
# The above should improve the extensibility and maintainability of Broadcast.

…unate Broadcast complexity combinatorial explosion edition).
@Sacha0 Sacha0 added sparse Sparse arrays broadcast Applying a function over a collection labels Jan 13, 2017
@Sacha0 Sacha0 changed the title issue-pr: extend sparse broadcast[!] to one- and two-dimensional arrays issue-pr: extend sparse broadcast[!] to one- and two-dimensional Arrays Jan 13, 2017
@martinholters
Copy link
Member

Could a mechanism like the for promote_type/promote_rule be used to keep the combinatorial explosion in check?

@nalimilan nalimilan self-assigned this Jan 13, 2017
promote_containertype(::Type{Matrix}, ::Type{Any}) = Matrix
promote_containertype(::Type{Any}, ::Type{Matrix}) = Matrix

promote_containertype(::Type{Vector}, ::Type{Matrix}) = Matrix
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't you replace definitions like this one with a general definition along the lines of this? Would it help fixing the problem?

promote_containertype{S<:Array, T<:Array}(::Type{S}, ::Type{T}) =
    Array{promote_type(eltype(S), eltype(T)), max(ndim(S), ndim(T))}`

(The element type isn't needed here of course, but I don't know how to create the Array{T, N} type without speciying T.)

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 13, 2017

Could a mechanism like the for promote_type/promote_rule be used to keep the combinatorial explosion in check?

Could you expand? promote_containertype is analogous to promote_type/promote_rule, except without the fallback typejoin logic and the mechanism that makes a single promote_rule(::S, ::T) definition sufficient (rather than an argument-exchange-symmetric pair). (Something like the latter mechanism would be nice and implementation should be straightforward, but would at best curb the explosion of necessary definitions by a factor of two?)

Couldn't you replace definitions like this one with a general definition along the lines of this? Would it help fixing the problem?

Though differentiating Arrays by dimension in Broadcast's containertype promotion mechanism makes life easier for consumers of Broadcast that must separate Arrays by dimension (e.g. sparse broadcast[!]), doing so forces all consumers of Broadcast to handle that additional complexity. Conjecturing that most consumers of Broadcast do not need to separate Arrays by dimension (?), and avoiding forcing additional complexity on the many outweighing the convenience of the few, I would advocate heading the other direction: not differentiating Arrays by dimension in Broadcast's containertype promotion mechanism, and instead requiring those few consumers of Broadcast that must separate Arrays by dimension to implement that logic internally (as in e.g. #20009). (If implementation works out, the changes I suggest above might make life easier for both groups, including the logic in #20009.)

Thanks and best!

@nalimilan
Copy link
Member

Makes sense. What's the problem then? :-)

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 14, 2017

What's the problem then? :-)

The comments in the second/terminating code block in the original post describe the (two) issues, though not eloquently. A little expansion below:

The first issue: Broadcast's containertype promotion mechanism uses Array to indicate any of a number of distinct concepts. For example, Array can simply mean "this object is an Array", but it can also mean "this is a collection including three different subtypes of AbstractArray, a pair of Tuples, and (for good measure) a Nullable". When extending Broadcast, distinguishing the former from the latter is sometimes necessary; this is the case with sparse broadcast, for example. To do so, you either need extend Broadcast's containertype promotion mechanism with additional types in a manner that impacts other consumers of Broadcast (as in this PR), or you need largely reimplement that mechanism (as in #20009).

The second issue: When teaching Broadcast about a new container type, Broadcast's existing containertype promotion fallbacks encourage doing so in a manner that forces other Broadcast-extenders to write methods taking that new container type into account. These lines illustrate this problem, showing the beginnings of a combinatorial explosion of disambiguating methods.

Best!

@tkelman
Copy link
Contributor

tkelman commented Jan 24, 2017

shall we close this one?

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 25, 2017

shall we close this one?

Sounds good!

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 sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants