Extend sparse broadcast to VecOrMats #20102

pabloferz · 2017-01-18T08:52:00Z

This is yet another alternative to #20007 and #20009, that also addresses #11474. The difference with the referred PRs is that, hopefully, this addresses @Sacha0's concerns pointed out on those PRs.

I have not included tests here as there are already proposed tests in the previous PRs which we can keep.

pabloferz · 2017-01-18T08:57:04Z

@nanosoldier runbenchmarks(ALL, vs=":master")

martinholters · 2017-01-18T11:06:34Z

Just an idea, not thought through to the end, so I don't know whether it's feasible, but could we use AbstractArray{T,N} where T for some specific N instead of AbstractArray and OneOrTwoD? E.g.

_containertype(::Type{T}) where (T<:AbstractArray{Te,N} where Te) where N = AbstractArray{Te,N} where Te

Then we wouldn't need ArrayType and could dispatch on Union{AbstractArray{Te,1}, AbstractArray{Te,2}} where Te instead of OneOrTwoD, which we also wouldn't need (but might want to keep as a shorter alias).

nanosoldier · 2017-01-18T13:08:17Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

pabloferz · 2017-01-18T15:52:33Z

@martinholters Actually, my first idea was to go with AbstractVecOrMat, but using AbstractArray{Te,N} where Te is more general and helps in case we need to specialize on more dimensions in the future. I'll change it.

pabloferz · 2017-01-20T00:34:08Z

@nanosoldier runbenchmarks(ALL, vs=":master")

pabloferz · 2017-01-20T00:39:43Z

@martinholters I tried to implement your idea and it felt somewhat convoluted, but I removed the OneOrTwoD type anyway. I propose we go with something like this for now, and if there is really a need of specializations for more dimensions in the future, only then we suck it up and handle the complexity.

pabloferz · 2017-01-20T00:42:48Z

I made a mistake above so here's again @nanosoldier runbenchmarks(ALL, vs=":master")

Sacha0 · 2017-01-20T03:53:26Z

base/sparse/higherorderfns.jl

@@ -861,8 +862,10 @@ broadcast_indices(::Type{AbstractSparseArray}, A) = indices(A)
 # broadcast container type promotion for combinations of sparse arrays and other types
 _containertype{T<:SparseVecOrMat}(::Type{T}) = AbstractSparseArray
 # combinations of sparse arrays with broadcast scalars should yield sparse arrays
-promote_containertype(::Type{Any}, ::Type{AbstractSparseArray}) = AbstractSparseArray
-promote_containertype(::Type{AbstractSparseArray}, ::Type{Any}) = AbstractSparseArray
+promote_containertype(::Type{AbstractSparseArray}, ::ScalarType) = AbstractSparseArray


I do not know whether sparse map[!]/broadcast[!] handles Nullables correctly. (Hence the absence of Nullables in the relevant code so far.) Have you confirmed that sparse map[!]/broadcast[!] handles Nullables correctly? Thanks!

It does not work, so I will remove it for now.

Sacha0 · 2017-01-20T04:01:58Z

base/sparse/higherorderfns.jl

+@inline _sparsifystructured(A::AbstractVector) = SparseVector(A)
+@inline _sparsifystructured(A::AbstractSparseMatrix) = SparseMatrixCSC(A)
+@inline _sparsifystructured(A::AbstractSparseVector) = SparseVector(A)
+@inline _sparsifystructured(S::SparseVecOrMat) = S


Good call. This combination of definitions (_sparsifystructured for AbstractSparseMatrix, AbstractSparseVector, and SparseVecOrMat) is safer than the definition that combination supplants (_sparsifystructured for AbstractSparseArray). We should make certain this change goes in one way or another. Thanks!

Sacha0 · 2017-01-20T04:10:12Z

base/sparse/higherorderfns.jl

@@ -871,11 +874,11 @@ promote_containertype(::Type{AbstractSparseArray}, ::Type{Tuple}) = Array

 # broadcast[!] entry points for combinations of sparse arrays and other (scalar) types
 @inline function broadcast_c{N}(f, ::Type{AbstractSparseArray}, mixedargs::Vararg{Any,N})
-    parevalf, passedargstup = capturescalars(f, mixedargs)
+    parevalf, passedargstup = capturescalars(f, map(_sparsifystructured, mixedargs))


Given the capturescalars pathway's standing shortcomings, sending sparse broadcast[!] calls through capturscalars that need not go through capturscalars makes me uneasy. (Overcoming capturescalars's shortcomings would be wonderful. Any thoughts on that front?)

I have added a capturedscalars method that should take care of this for now until we find a better alternative (I have thought of having a method that iterates over columns for AbstractArrays in general and doing something like _broadcast! with _broadcast_getindex! so we don't need this mechanism, but that would have to wait)

I have thought of having a method that iterates over columns for AbstractArrays in general and doing something like _broadcast! with _broadcast_getindex! so we don't need this mechanism

Could you clarify? If you mean something along the lines of extending the common interface over SparseVectors and SparseMatrixCSCs in sparse broadcast[!] to other types (e.g. scalars, thereby obviating the capturescalars mechanism), then that matches my plan for evolving these as well.

Yeah, a mechanism for avoiding the need of capturescalars (similar to the way things work for Arrays) is what I meant, but we can discuss that later.

Sacha0

Much thanks for working on this @pabloferz! If I'm not mistaken, this version will not correctly funnel certain argument combinations to sparse broadcast[!]. For example, suppose you have an argument tuple including scalars, Vectors, and SparseMatrixCSCs. For certain argument orderings, it seems promote_containertype may receive e.g. an Any-VecOrMat pair and return Array. Downstream the broadcast[!] operation will then hit AbstractArray broadcast[!] rather than sparse broadcast[!]. (This issue is part of why disambiguating Array in Broadcast's containertype promotion mechanism may be worthwhile.) To properly handle such cases, you need additional machinery to capture combinations identified as Arrays by Broadcast's containertype promotion mechanism and examine them more critically to determine whether sparse broadcast[!] should handle them or not (as in e.g. #20009; #20007 takes a different approach). Best!

Sacha0 · 2017-01-20T04:39:43Z

hopefully, this addresses @Sacha0's concerns pointed out on those PRs

Partially! :) On the one hand, replacing Broadcast's promote_containertype(::Type{Array}, ct) = Array fallback pair with promote_containertype(ct, ct) = Array seems advantageous as discussed in #20007. On the other hand, adding VecOrMat to Broadcast's containertype promotion mechanism might be suboptimal as discussed in the second paragraph of #20007 (comment). (Avoiding that expansion was #20009's purpose.)

Over the last few days I've thought through the issues #20007 hightlights. I hope to write those thoughts out in the next few days; please look for more from me soon. Thanks again!

nanosoldier · 2017-01-20T04:49:39Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

pabloferz · 2017-01-20T22:34:04Z

If I'm not mistaken, this version will not correctly funnel certain argument combinations to sparse broadcast[!]. For example, suppose you have an argument tuple including scalars, Vectors, and SparseMatrixCSCs. For certain argument orderings, it seems promote_containertype may receive e.g. an Any-VecOrMat pair and return Array.

Fixed

adding VecOrMat to Broadcast's containertype promotion mechanism might be suboptimal as discussed in the second paragraph of #20007 (comment).

Now, if you don't need to distinguish between VecOrMat and Array, you only need to define promote_containertype for your own type and Type{T} where T<:Array (the same goes for AbstractSparseArray and StructuredArray, i.e. to handle both equally, just define promote_containertype for Type{T} where T<:AbstractSparseArray and your own type).

pabloferz · 2017-01-20T22:39:24Z

Also, this uncovered a bug on calls to _unchecked_maxnnzbcres which is now fixed.

pabloferz · 2017-01-21T05:37:54Z

@JeffBezanson @vtjnash I'm seeing some of the #11840 tests failing here, do you have any idea why?

Sacha0 · 2017-01-21T20:20:14Z

Also, this uncovered a bug on calls to _unchecked_maxnnzbcres which is now fixed.

#19986 (pending merge) fixes this issue. Best!

pabloferz · 2017-01-21T20:31:18Z

#19986 (pending merge) fixes this issue. Best!

I had missed that one. I'd say that should go in first and I'll rebase on top of it.

Sacha0 · 2017-01-22T18:41:43Z

The latest iteration suffers from other correctness issues unfortunately. And as I conveyed gently above, I have misgivings about the direction in which this pull request evolves broadcast[!]'s containertype promotion mechanism. (This pull request is equivalent to an iteration of #20007 simplified with benefit of #20007's promote_containertype(ct, ct) suggestion, but intensifying the ambiguity and special-casing issues #20007 identifies.)

How to best structure broadcast[!]'s containertype promotion mechanism deserves careful thought. I look forward to putting our heads together to design a good long-term solution. My development and review time is limited till the end of February (neglected other work while pushing for feature freeze, now have to clear the accumulation), and there are several things I would like to dedicate time to prior to the release. That being the case, an ultimately better design being more important than rushing something into 0.6, and having a correct, nondisruptive implementation that both provides the desired functionality and confines additional complexity (#20009), I would prefer that for now we spend the limited remaining time on, e.g., fixing bugs, writing docs, and tying up other loose ends for the release (and perhaps the above design work, time allowing), rather than iterating hastily here.

Thoughts? Thanks and best!

martinholters · 2017-01-23T10:40:14Z

Not related to this PR (only), but in response to @Sacha0: I think post 0.6, it is worth collecting all the lessons learned documented in various issues and PRs and rethink the whole broadcast-related code (and probably map! along with it) with the objective of having some basic building blocks that can be better reused and help getting all corner-cases right. But that's definitely not something to rush in...

pabloferz · 2017-01-23T16:31:37Z

The latest iteration suffers from other correctness issues unfortunately.

If you have the time, could you exemplify what would fail with this approach (to take it into account in the future)?

With respect to not modifying the containertype logic too much for 0.6, I don't mind. That is, if you want to merge #20009 for now and leave discussions on this end for the next cycle, that would be fine.

I think post 0.6, it is worth collecting all the lessons learned documented in various issues and PRs and rethink the whole broadcast-related code (and probably map! along with it) with the objective of having some basic building blocks that can be better reused and help getting all corner-cases right.

@martinholters That's an excellent idea and I look forward to doing this 👍

Sacha0 · 2017-01-25T03:57:00Z

If you have the time, could you exemplify what would fail with this approach (to take it into account in the future)?

Absolutely. The concerns I recall immediately from when I skimmed this iteration are:

The promote_containertype(::Type{T}, ::ScalarType) definitions would allow combinations involving Nullables to funnel to sparse broadcast[!], wouldn't they? (Additionally, these definitions compound the ambiguity of the meaning of types used in the container type promotion mechanism, don't they? That ambiguity makes behavior difficult to reason about, which is one of the issues highlighted in #20007.)

The containertype{T<:AbstractVecOrMat}(::Type{T}) = VecOrMat definition seems too broad: First VecOrMat becomes ambiguous, and then what exactly gets funneled to sparse broadcast[!] becomes ambiguous as a result. Generally, only funneling type combinations to sparse broadcast[!] that we know it can handle (while defaulting everything else to AbstractArray broadcast[!]) seems a safer approach.

If I'm not mistaken, this iteration funnels some combinations that do not involve scalars but do involve structured matrices through capturescalars (unnecessarily).

Some additional concern regarding collapsing broadcast_c!'s second and third arguments (destination container type, source combination container type) that I cannot recall. I like the simplification, but IIUC I separated those arguments for a reason. But perhaps not :).

@martinholters That's an excellent idea and I look forward to doing this 👍

Seconded! Best!

pabloferz · 2017-10-03T19:45:06Z

Closing in favor of #23939

pabloferz added domain:arrays:sparse Sparse arrays domain:broadcast Applying a function over a collection labels Jan 18, 2017

pabloferz force-pushed the pz/morebc branch from d6e85e8 to 477e1d8 Compare January 20, 2017 00:33

pabloferz requested a review from Sacha0 January 20, 2017 00:44

Sacha0 reviewed Jan 20, 2017

View reviewed changes

pabloferz force-pushed the pz/morebc branch 2 times, most recently from f513300 to 5c571aa Compare January 20, 2017 21:59

pabloferz force-pushed the pz/morebc branch from 5c571aa to aa8d3a1 Compare January 21, 2017 00:58

pabloferz force-pushed the pz/morebc branch from aa8d3a1 to 039b793 Compare January 21, 2017 18:56

pabloferz force-pushed the pz/morebc branch from 039b793 to 7e35d6f Compare January 21, 2017 23:21

pabloferz mentioned this pull request Jan 22, 2017

broadcast for AbstractArray types #17533

Open

Sacha0 mentioned this pull request Feb 22, 2017

Wishlist: API and documentation for extending broadcast #20740

Closed

pabloferz force-pushed the pz/morebc branch from 7e35d6f to de1b603 Compare March 9, 2017 01:48

pabloferz mentioned this pull request Mar 9, 2017

sparse .* rowvec is dense #20954

Closed

Extend sparse broadcast to VecOrMats

3258952

pabloferz force-pushed the pz/morebc branch from de1b603 to 3258952 Compare March 9, 2017 02:14

pabloferz mentioned this pull request Mar 9, 2017

Make broadcast over sparse and rowvecs sparse #20959

Merged

pabloferz closed this Oct 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend sparse broadcast to VecOrMats #20102

Extend sparse broadcast to VecOrMats #20102

pabloferz commented Jan 18, 2017

pabloferz commented Jan 18, 2017

martinholters commented Jan 18, 2017 •

edited

Loading

nanosoldier commented Jan 18, 2017

pabloferz commented Jan 18, 2017

pabloferz commented Jan 20, 2017 •

edited

Loading

pabloferz commented Jan 20, 2017

pabloferz commented Jan 20, 2017

Sacha0 Jan 20, 2017

pabloferz Jan 20, 2017

Sacha0 Jan 20, 2017

Sacha0 Jan 20, 2017

pabloferz Jan 20, 2017

Sacha0 Jan 21, 2017

pabloferz Jan 21, 2017

Sacha0 left a comment

Sacha0 commented Jan 20, 2017

nanosoldier commented Jan 20, 2017

pabloferz commented Jan 20, 2017 •

edited

Loading

pabloferz commented Jan 20, 2017 •

edited

Loading

pabloferz commented Jan 21, 2017 •

edited

Loading

Sacha0 commented Jan 21, 2017

pabloferz commented Jan 21, 2017

Sacha0 commented Jan 22, 2017

martinholters commented Jan 23, 2017

pabloferz commented Jan 23, 2017

Sacha0 commented Jan 25, 2017 •

edited

Loading

pabloferz commented Oct 3, 2017

Extend sparse broadcast to VecOrMats #20102

Extend sparse broadcast to VecOrMats #20102

Conversation

pabloferz commented Jan 18, 2017

pabloferz commented Jan 18, 2017

martinholters commented Jan 18, 2017 • edited Loading

nanosoldier commented Jan 18, 2017

pabloferz commented Jan 18, 2017

pabloferz commented Jan 20, 2017 • edited Loading

pabloferz commented Jan 20, 2017

pabloferz commented Jan 20, 2017

Sacha0 Jan 20, 2017

Choose a reason for hiding this comment

pabloferz Jan 20, 2017

Choose a reason for hiding this comment

Sacha0 Jan 20, 2017

Choose a reason for hiding this comment

Sacha0 Jan 20, 2017

Choose a reason for hiding this comment

pabloferz Jan 20, 2017

Choose a reason for hiding this comment

Sacha0 Jan 21, 2017

Choose a reason for hiding this comment

pabloferz Jan 21, 2017

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

Sacha0 commented Jan 20, 2017

nanosoldier commented Jan 20, 2017

pabloferz commented Jan 20, 2017 • edited Loading

pabloferz commented Jan 20, 2017 • edited Loading

pabloferz commented Jan 21, 2017 • edited Loading

Sacha0 commented Jan 21, 2017

pabloferz commented Jan 21, 2017

Sacha0 commented Jan 22, 2017

martinholters commented Jan 23, 2017

pabloferz commented Jan 23, 2017

Sacha0 commented Jan 25, 2017 • edited Loading

pabloferz commented Oct 3, 2017

martinholters commented Jan 18, 2017 •

edited

Loading

pabloferz commented Jan 20, 2017 •

edited

Loading

pabloferz commented Jan 20, 2017 •

edited

Loading

pabloferz commented Jan 20, 2017 •

edited

Loading

pabloferz commented Jan 21, 2017 •

edited

Loading

Sacha0 commented Jan 25, 2017 •

edited

Loading