fix #28369, transpose of SparseMatrixCSC is not recursive

[jlapeyre]

No description provided.

[Sacha0]

Did you perhaps mean to test permutedims(S, (1,2)) == M instead?

[jlapeyre]

permutedims(M, (1,2)) == S doesn't test the new code. permutedims(S, (1,2)) == M is definitely better.

[jlapeyre]

I fixed this and rebased and pushed the PR again.

[jlapeyre]

This line is redundant. It is covered by the permutedims(A::AbstractMatrix) = permutedims(A, (2,1) in permuteddimsarray.jl

[jlapeyre]

I fixed this and rebased and pushed the PR again

[Sacha0]

A nanosoldier run might've been in order, to make certain that the replacement of the function passed to ftranspose did not introduce any performance regressions?

[andreasnoack]

Another time please just go ahead and trigger a run.

@nanosoldier runbenchmarks("array", vs = "@c05fd201baedb6681947cc212e0a9f49564d1568")

[jlapeyre]

Is this something I can do locally ?

[andreasnoack]

Yes, you can run the same tests locally. See https://github.com/JuliaCI/BaseBenchmarks.jl. Though, it might be easier to ask a collaborator to trigger a nanosoldier run.

[nanosoldier]

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

[jlapeyre]

There appear to be statistically significant regressions (+ 30-40%) in some sparse benchmarks. I'll look at this.

[andreasnoack]

I'm not convinced that any of these are genuine but it would be great if you could take a closer look anyway

[jlapeyre]

I misread the results. Time increases of about %30 are precisely on those with about %30 tolerance.

But, there is an oversight in the PR.
transpose! and adjoint! should also be recursive.

julia/stdlib/SparseArrays/src/sparsematrix.jl

Lines 858 to 859 in b0bf91e

	transpose!(X::SparseMatrixCSC{Tv,Ti}, A::SparseMatrixCSC{Tv,Ti}) where {Tv,Ti} = ftranspose!(X, A, identity)
	adjoint!(X::SparseMatrixCSC{Tv,Ti}, A::SparseMatrixCSC{Tv,Ti}) where {Tv,Ti} = ftranspose!(X, A, conj)

[Sacha0]

The collection of sparse-dense and dense-sparse matrix multiplication regressions look potentially legitimate; that they occur as a block makes the likelihood that those represent a real change reasonable. Perhaps check those locally?

[jlapeyre]

EDIT: Please see the comment below.

It looks like doing this correctly will take a bit more work. As someone mentioned, the sparsematrix.jl code relies in a few places on the elements being scalars.

Suppose
S = sparse(reshape([[1 2; 3 4], [9 10; 11 12], [5 6; 7 8], [13 14; 15 16]], (2,2))).

One problem is unnecessary allocation. copy(transpose(S)) calls ftranspose, which allocates a container

julia/stdlib/SparseArrays/src/sparsematrix.jl

Lines 861 to 865 in 883c8a3

	function ftranspose(A::SparseMatrixCSC{Tv,Ti}, f::Function) where {Tv,Ti}
	X = SparseMatrixCSC(A.n, A.m,
	Vector{Ti}(undef, A.m+1),
	Vector{Ti}(undef, nnz(A)),
	Vector{Tv}(undef, nnz(A)))

Eventually, _distributevals_halfperm! is called, which rebinds the top-level elements in the allocated container to the (materially) transposed sub-matrices.

This PR neglected to fix tranpose!. If we fix it by again replacing identity with x -> copy(tranpose(x)) here

julia/stdlib/SparseArrays/src/sparsematrix.jl

Line 858 in 883c8a3

transpose!(X::SparseMatrixCSC{Tv,Ti}, A::SparseMatrixCSC{Tv,Ti}) where {Tv,Ti} = ftranspose!(X, A, identity)

the same additional allocation will happen.

But, I haven't done or tested anything yet because Revise is broken for the moment on Julia master. I guess that redundant allocation is preferable in the short term to silently giving the wrong result.