deprecate spones#25037
Conversation
fredrikekre
added
deprecation
e13e4ce to
a0ada0f
Compare
| respectively. The new `spdiagm` implementation now always returns a square matrix ([#23757]). | ||
|
|
||
| * `spones(A::AbstractSparseArray{T})` has been deprecated in favor of | ||
| `LinAlg.fillstored!(copy(A), one(T))` ([#TBD]). |
There was a problem hiding this comment.
The one(T) could simplify to 1 in the vast majority of cases, as fillstored! necessarily converts that 1 to eltype(A)? Suggesting the simplest replacements first is nice :).
|
|
||
| # PR #TBD | ||
| @eval SparseArrays @deprecate spones(A::SparseMatrixCSC{T}) where {T} fillstored!(copy(A), one(T)) | ||
| @eval SparseArrays @deprecate spones(A::SparseVector{T}) where {T} fillstored!(copy(A), one(T)) |
There was a problem hiding this comment.
Likewise here, perhaps simplify the suggested replacement as much as possible?
| """ | ||
| spones(S::SparseMatrixCSC{T}) where {T} = | ||
| SparseMatrixCSC(S.m, S.n, copy(S.colptr), copy(S.rowval), ones(T, S.colptr[end]-1)) | ||
| LinAlg.fillstored!(S::SparseMatrixCSC, x) = (fill!(S.nzval, x); S) |
There was a problem hiding this comment.
fill!(view(S.nzval, 1:(S.colptr[S.n + 1] - 1)), x)?
| export AbstractSparseArray, AbstractSparseMatrix, AbstractSparseVector, | ||
| SparseMatrixCSC, SparseVector, blkdiag, droptol!, dropzeros!, dropzeros, | ||
| issparse, nonzeros, nzrange, rowvals, sparse, sparsevec, spdiagm, spones, | ||
| issparse, nonzeros, nzrange, rowvals, sparse, sparsevec, spdiagm, fillstored!, |
There was a problem hiding this comment.
Why export fillstored! from SparseArrays while LinAlg owns fillstored!?
|
|
||
| # Construction of same structure, but with all ones | ||
| spones(x::SparseVector{T}) where {T} = SparseVector(x.n, copy(x.nzind), ones(T, length(x.nzval))) | ||
| LinAlg.fillstored!(x::SparseVector{T}, y) where {T} = (fill!(x.nzval, y); x) |
There was a problem hiding this comment.
Perhaps nix the unnecessary type parameter?
| @testset "spones" begin | ||
| @test spones(sparse(2.0I, 5, 5)) == Matrix(I, 5, 5) | ||
| @testset "fillstored!" begin | ||
| @test SparseArrays.fillstored!(sparse(2.0I, 5, 5), 1) == Matrix(I, 5, 5) |
There was a problem hiding this comment.
Why the SparseArrays qualification when the definitions in SparseArrays extend the name from LinAlg? (Likewise below :).)
|
Much thanks for clearing this one @fredrikekre! :) |
bb8f554 to
9ec0111
Compare
|
Rebased, planning to merge this when ci is done. |
| # PR #25030 | ||
| @eval LinAlg @deprecate fillslots! fillstored! false | ||
|
|
||
| # PR #TBD |
There was a problem hiding this comment.
Todo! the CI jobs to seem to not have started yet, so it's still time to update this without having CI resources wasted.
There was a problem hiding this comment.
In the future maybe CI will check for #TBD #TODO, [#CATS] etc!
|
I may have broken the build right when this was pushed, so probably worth fixing and rebasing. |
9ec0111 to
ddd292d
Compare
ddd292d to
c4b83e0
Compare
|
Nevermind, I fixed it and pushed it for you. |
|
I think we did the same thing at the same time... |
|
I'm so confused... I only fixed the missing PR number that Rafael pointed out :) |
c4b83e0 to
462ee64
Compare
|
Ok, I did that plus I rebased on top of a newer master where I fixed the merge mistake. |
|
The i686 AV linalg/bunchkaufman failure is identical to that in #25083. Seems something broke AV i686 linalg/bunchkaufman recently but was nonetheless merged? Having a look at the AV queue... (Edit: Seems AV was out of commission for a bit, during which period the AV i686 linalg/bunchkaufman breaking change likely went through innocently.) |
Sacha0
left a comment
Sacha0
left a comment
There was a problem hiding this comment.
lgtm for the most recent version! Thanks @fredrikekre! :)
4 participants
This is a rather useless function.
This is on top of #25030, relevant commit for this PR is a0ada0f
fix #22381