Undeprecate real and imag for sparse matrices #22090
Conversation
|
Hmm... This does not quite restore the 0.5 behaviour. On 0.5 julia/base/abstractarraymath.jl Line 88 in c3b8ad0 A::AbstractArray{<:Real} but a copy (julia/base/sparse/sparsematrix.jl Line 1356 in c3b8ad0 ::SparseMatrix{<:Real}. That seems like a bug?
|
|
where is the fallback for this? should test that at least for complex arrays where this should return a new array, the index arrays aren't aliasing, for SparseMatrixCSC or SparseVector |
|
Well, that issue is automatically fixed by this PR currently since sparse matrix will dispatch to julia/base/abstractarraymath.jl Line 90 in c3fb8e7 On the other hand, |
kshyatt
added
kind:deprecation
not for complex elements |
|
I think the behaviour is correct now, will add tests. |
base/sparse/sparsematrix.jl
Outdated
| @@ -1447,6 +1447,8 @@ sparse(S::UniformScaling, m::Integer, n::Integer=m) = speye_scaled(S.λ, m, n) | |||
| conj!(A::SparseMatrixCSC) = (@inbounds broadcast!(conj, A.nzval, A.nzval); A) | |||
| (-)(A::SparseMatrixCSC) = SparseMatrixCSC(A.m, A.n, copy(A.colptr), copy(A.rowval), map(-, A.nzval)) | |||
|
|
|||
| # the rest of real, conj, imag are handled correctly via AbstractArray methods | |||
| imag(A::SparseMatrixCSC{Tv,Ti}) where {Tv<:Number,Ti} = spzeros(Tv, Ti, A.m, A.n) | |||
There was a problem hiding this comment.
wouldn't SparseVector need the same?
There was a problem hiding this comment.
julia/base/sparse/sparsevector.jl
Line 1025 in 7a05f68
Seems like there has been some inconsistency here for
AbstractArray/SparseMatrixCSC/SparseVector.
fredrikekre
force-pushed
the
fe/real-imag-sparse
branch
from
a521427
to
9d8f105
Compare
| @@ -985,7 +985,6 @@ hvcat(rows::Tuple{Vararg{Int}}, xs::_TypedDenseConcatGroup{T}...) where {T} = Ba | |||
| ### Unary Map | |||
|
|
|||
| # zero-preserving functions (z->z, nz->nz) | |||
| conj(x::SparseVector) = SparseVector(length(x), copy(nonzeroinds(x)), conj(nonzeros(x))) | |||
There was a problem hiding this comment.
this may have been kept as a separate method if it was faster than broadcast
There was a problem hiding this comment.
Should be faster, if marginally. Worth the extra complexity though?
There was a problem hiding this comment.
it's one line, so if it makes more than say 10-15% difference, imo yes
There was a problem hiding this comment.
Didn't think about that, I will add it back
There was a problem hiding this comment.
(Checked performance difference, slightly under a factor of two in a simple test case.)
There was a problem hiding this comment.
Might as well add such a shortcut for SparseMatrixCSC also?
There was a problem hiding this comment.
if it doesn't have one, might be (and worth tracking along with other sparse broadcast perf in case it gets better someday and this stops being as much of a difference)
fredrikekre
force-pushed
the
fe/real-imag-sparse
branch
from
315cc56
to
2f4ae4b
Compare
make sure aliasing is the same as for AbstracArrays for SparseArray{<:Real}
add tests for this
add shortcut for conj(SparseArray{<:Complex} which is faster than broadcasting
fredrikekre
force-pushed
the
fe/real-imag-sparse
branch
from
2f4ae4b
to
11b9d81
Compare
make sure aliasing is the same as for AbstracArrays for SparseArray{<:Real}
add tests for this
add shortcut for conj(SparseArray{<:Complex} which is faster than broadcasting
(cherry picked from commit 6b0d54e)
fix #22065
ref #18566, #18495 (comment)
conjgot the same treatment in #19242cc @Sacha0