Simplify nz2nz_z2z-class sparse unary broadcast (and fix #18974)

[Sacha0]

Following @stevengj's suggestion in #18975, this pull request makes nz2nz_z2z-class sparse unary broadcast leverage existing broadcast machinery rather than reimplement it. Also fixes #18974.

This implementation is slightly slower than the existing implementation due to the view in Bnzval = broadcast(f, view(A.nzval, 1:nnz(A))); removing the view (Bnzval = broadcast(f, A.nzval)) eliminates the performance gap. Bnzval = broadcast(f, A.nzval); resize!(Bnzval, nnz(A)) might be a reasonable alternative. For discussion of performance (and particularly the apparent need for forcing function-type specialization), see #18975. Best!

[tkelman]

is the view necessary? nzval is only ever not that length if the input data is invalid, right?

[Sacha0]

is the view necessary?

Not strictly, no. The view preserves the method's existing behavior, but so would (Bnzval = broadcast(f, A.nzval); resize!(Bnzval, nnz(A))). Preserving the method's existing behavior also doesn't seem strictly necessary?

nzval is only ever not that length if the input data is invalid, right?

length(A.nzval) > nnz(A) is valid I believe?

Thanks for reviewing! Best!

[tkelman]

You can have extra space in the nzval array, but there may be places that use nnz and length(nzval) interchangeably without checking whether they're the same.

[stevengj]

The resize! approach seems fine to me; in common cases it will be a no-op.

[Sacha0]

Switched to the resize! approach. Thanks!

[stevengj]

Did you mean to delete the specialized broadcast methods for abs and abs2 on sparse matrices?

(Eventually I think these should go away in favor of a more general mechanism, but that seems like a matter for another PR?)

[Sacha0]

The deletion was intentional: The (revised) unspecialized broadcast method appears to provide the behavior of the specializations for abs and abs2 (per a little testing at the REPL). More than happy to restore those specializations and nix them elsewhere though if you prefer? Thanks for reviewing!

[stevengj]

I'm confused, was there already a PR that updated the unspecialized broadcast for unary operations on sparse arrays?

[Sacha0]

Apologies for the confusion. Clarification: The macro call

 @_enumerate_childmethods(_broadcast_unary_nz2nz_z2z,
     log1p, expm1, abs, abs2, conj)

defines methods

broadcast(::typeof(abs), A::SparseMatrixCSC) = _broadcast_unary_nz2nz_z2z(abs, A)
broadcast(::typeof(abs2), A::SparseMatrixCSC) = _broadcast_unary_nz2nz_z2z(abs2, A)

which, with _broadcast_unary_nz2nz_z2z using Bnzval = broadcast(f, A.nzval) to determine output type on this branch, seem to provide the behavior of the removed methods,

broadcast{TTv}(::typeof(abs2), A::SparseMatrixCSC{Complex{TTv}}) = _broadcast_unary_nz2nz_z2z_T(abs2, A, TTv)
broadcast{TTv}(::typeof(abs), A::SparseMatrixCSC{Complex{TTv}}) = _broadcast_unary_nz2nz_z2z_T(abs, A, TTv)
broadcast{TTv<:Integer}(::typeof(abs), A::SparseMatrixCSC{Complex{TTv}}) = broadcast_unary_nz2nz_z2z_T(abs, A, Float64)
broadcast{TTv<:BigInt}(::typeof(abs), A::SparseMatrixCSC{Complex{TTv}}) = broadcast_unary_nz2nz_z2z_T(abs, A, BigFloat)

Does that clarify? Best!

[stevengj]

Ah good.

[stevengj]

PS. Can you file a performance issue with a test case illustrating the slowdown if you omit the <:Function signature? That really needs to be fixed...

[Sacha0]

Can you file a performance issue with a test case illustrating the slowdown if you omit the <:Function signature? That really needs to be fixed...

Shall do, likely tomorrow morning Done. Thanks!

[tkelman]

dunno if we have benchmarks for this, but may as well @nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

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

[Sacha0]

The possible performance regressions look like noise.

[stevengj]

Yup, they are not things that could have been affected by this.

[Sacha0]

Thanks for reviewing / merging!