sparseconvert: copy falls back to copymutable for non-sparse SubArray

[KlausC]

Fixes #32213

[fredrikekre]

Why is this method needed at all?

[KlausC]

@fredrikekre, in the case of views of sparse matrices, there is an essential performance improvement compared to the generic Base.copymutable implementation possible (for example factor 800 for n=10^3, nnz=10^4).

julia> n = 1000
1000
julia> A = sprand(n, n, 10^4/n^2)
1000×1000 SparseMatrixCSC{Float64,Int64} with 10166 stored entries:
...
julia> B = view((A), 1:999, 1:999);

julia> @btime copy($B);
  56.902 μs (6 allocations: 166.64 KiB)

julia> @btime Base.copymutable($B);
  46.470 ms (8 allocations: 325.92 KiB)

[KristofferC]

Instead of type piracing this on all arrays, can't it be restricted to only apply to the sparse arrays we have?

[KlausC]

@KristofferC, it could be restricted to a finite number of cases by union or unionall type restrictions, but not to the complete infinite set of nested wrapped sparse matrices. That was the intention of merged PR #30552, which includes the now fixed definition of copy.
Because I had the impression, that the approach in this PR is somhow smelly, I made an alternative approach in PR #31563, which would allow to identify the mentioned types by an additional type parameter and amendment of the type hierarchy (AbstractArray :> AbstractWrappedArray :> SubArray). Once the second PR, or similar functionality, would be accepted, the cases of type piracy in the temporary approach could be avoided completely.

[fredrikekre]

@fredrikekre, in the case of views of sparse matrices, there is an essential performance improvement compared to the generic Base.copymutable implementation possible

Yea, I get that, but (i) I don't see how that relates to the changes in #30552. Where is copy called? and (ii) do we really need a special copy method for views of transposes/adjoints/etc of sparsematrices?

[KlausC]

@fredrikekre, I try to remember my motivation to propose this method as part of #30552.

(i) I don't see how that relates to the changes in #30552. Where is copy called?

I recognized, that there are specialized methods of copy for most other wrapper types, except SubArray, which make use of the sparsity of its argument. I wanted to extend the performance improvements to all kinds of wrapped sparse matrices. copy is called by the user and not needed for the implementation of the rest of #30552.

(ii) do we really need a special copy method for views of transposes/adjoints/etc of sparsematrices?

I don't know. It is just an attempt to prevent the user from unexpectedly worse operation times.

[fredrikekre]

Okay. Can I suggest that we completely drop this copy method for now and we can open a PR/issue where copy(::SubArray) is discussed? Seems like the most sane option since it was an unrelated change and presumably no-one really reviewed.

Edit: #32266

[KlausC]

understood.