fix and then specialize sparse broadcast!(f, C, A), strengthen tests #19986
Conversation
Sacha0
added
bugfix
| @test broadcast!(sin, copy(A), A) == sparse(broadcast!(sin, copy(fA), fA)) | ||
| # The two following tests are from #19895, and check the absence of an ambiguity | ||
| # between a pair of specializations, but #19895 removed both of those specializations, | ||
| # so perhaps these tests should be nixed? |
There was a problem hiding this comment.
@martinholters @pabloferz, thoughts? Thanks!
There was a problem hiding this comment.
A few months down the road, someone decides to add specializations for the common x .= y case to be faster...
There was a problem hiding this comment.
On that note, perhaps we should add tests for #19895 (comment). Will add a commit. Thanks!
| # We first divide the cases into two groups: those in which the input argument does not | ||
| # expand vertically, and those in which the input argument expands vertically. | ||
| # | ||
| # Cases without version expansion |
There was a problem hiding this comment.
Good catch. Fixed on push. Thanks!
| # if fofAx is zero, then either A's jth column is empty, or A's jth column | ||
| # contains a nonzero value x but f(Ax) is nonetheless zero, so we need store | ||
| # nothing in C's jth column. if to the contrary fofAx is nonzero, then we must | ||
| # densely populate C's jth column wiht fofAx. |
There was a problem hiding this comment.
Yesterday was not a good day for spelling. Fixed on push. Thanks!
| _densestructure!(C) | ||
| # Populate values | ||
| fill!(storedvals(C), fillvalue) | ||
| # Cases without version expansion |
| # above. here we simply lightly exercise the relevant broadcast[!] entry points. | ||
| @testset "broadcast implementation specialized for a single (input) sparse vector/matrix" begin | ||
| # broadcast for a single (input) sparse vector/matrix falls back to map, tested | ||
| # extensively above. hre we simply lightly exercise the relevant broadcast entry |
Sacha0
force-pushed
the
threeargbcbang
branch
from
934939c to
bec0c0d
Compare
|
Added a commit strengthening |
|
Absent objections or requests for time, I plan to merge this Thursday morning PST. Best! |
…rengthen associated tests.
…-preserving and tighten related tests.
…uction of erroneous or ambiguous specializations.
Sacha0
force-pushed
the
threeargbcbang
branch
from
bec0c0d to
5fcc60e
Compare
|
LGTM |
|
Thanks for reviewing / merging! |
In #19895 (comment), @martinholters (thanks!) noticed that the existing sparse
broadcast!(f, C, A)specialization does not handle cases where the shapes of destinationCand sourceAdiffer.This pull request's first commit removes the incorrect sparse
broadcast!(f, C, A)specialization (falling back to the argument-number-generic sparsebroadcast!code, which does not suffer from the above issue) and strengthens associated tests. The argument-number-generic code leaves some performance on the table, so the second commit provides a corrected sparsebroadcast!(f, C, A)specialization for zero-preservingf, and the third commit likewise for not-zero-preservingf.While writing this pull request I realized that the sparse
broadcast!(f, C, A, B)specializations suffer from the same issue. A fix for that code is in the works. Best!