Remove explicit dependence of sparse broadcast on type inference #19623
Conversation
|
Do our BaseBenchmarks exercise any of this? On first glance, I could not even find basic math operations on sparse matrices. So probably no point running nanosoldier? Anyway, I just did |
| _broadcast_type(f, As...) = Base._promote_op(f, Base.Broadcast.typestuple(As...)) | ||
| @inline _update_nzval!{T}(nzval::Vector{T}, k, x::T) = (nzval[k] = x; nzval) | ||
| @inline function _update_nzval!{T,Tx}(nzval::Vector{T}, k, x::Tx) | ||
| nzval = convert(Vector{typejoin(Tx, T)}, nzval) |
There was a problem hiding this comment.
I think if you use a different local var name for this the compiler may have a better time
Instead of determining the output element type beforehand by querying inference, the element type is deduced from the actually computed output values (similar to broadcast over Array, but taking into account the output for the all-inputs-zero case). For the type-unstable case, performance is sub-optimal, but at least it gives the correct result. Closes #19595.
martinholters
force-pushed
the
mh/fix_19595
branch
from
0f341d6 to
afae1e6
Compare
|
kind of a fiddly trick, but if it works... @Sacha0 what are your thoughts on this one? |
|
Unfortunately this implementation can silently return incorrect results and may degrade performance in some cases. I have a plan for further improving sparse |
Unfortunately not at this time. I have a suite of benchmarks with which I've been testing as I go that cover a broad array of cases (cartesian product of [matrix order of 10^3 and 10^5], [3, 10, and 100 stored entries per row/column], [same el and ind types, different eltype, different indtype, different el and ind types], [all shape combinations relevant to sparse broadcast over two input matrices]). Wrapping those into a BaseBenchmarks PR is in the queue but likely a few weeks out. Best! |
Oh. Do you have an example at hand? |
Yes, for example julia> foo = sparse(1:5, 1:5, 1:5)
5×5 sparse matrix with 5 Int64 nonzero entries:
[1, 1] = 1
[2, 2] = 2
[3, 3] = 3
[4, 4] = 4
[5, 5] = 5
julia> bar = copy(foo)
5×5 sparse matrix with 5 Int64 nonzero entries:
[1, 1] = 1
[2, 2] = 2
[3, 3] = 3
[4, 4] = 4
[5, 5] = 5
julia> map!(x -> x * 2., bar, foo)
5×5 sparse matrix with 5 Real nonzero entries:
[1, 1] = 2.0
[2, 2] = 4.0
[3, 3] = 6.0
[4, 4] = 8.0
[5, 5] = 10.0
julia> bar
5×5 sparse matrix with 5 Int64 nonzero entries:
[1, 1] = 1
[2, 2] = 2
[3, 3] = 3
[4, 4] = 4
[5, 5] = 5Best! |
|
Ah, of course, I had not thought about the |
|
Closing as I probably won't be making any progress here before some time in January and I guess (hope) this will be made obsolete by @Sacha0's ongoing work by then... |
For the type-stable case (i.e. the function
fbeingbroadcast/mapped can be inferred to give a leaf type), the compiler should be able to elide any overhead introduced by this, so I expect performance not to suffer.For the type-unstable case, improvements are certainly possible performance-wise, but this should at least get things in a working state again.
Replaces #19611, closes #19595.
Note that #19589 is still needed in principle, although this PR also makes it less likely to hit #19561. I.e. the examples therein work with this PR, but the underlying problem is only solved in #19589.