Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deprecate spones #25037

Merged
merged 1 commit into from
Dec 15, 2017
Merged

deprecate spones #25037

merged 1 commit into from
Dec 15, 2017

Conversation

fredrikekre
Copy link
Member

@fredrikekre fredrikekre commented Dec 12, 2017 •
edited
Loading

This is a rather useless function.

This is on top of #25030, relevant commit for this PR is a0ada0f
fix #22381

@fredrikekre fredrikekre added deprecation This change introduces or involves a deprecation sparse Sparse arrays labels Dec 12, 2017
@fredrikekre fredrikekre requested a review from Sacha0 December 12, 2017 08:31
Sacha0
Sacha0 reviewed Dec 12, 2017
View reviewed changes
NEWS.md Outdated
@@ -704,6 +704,9 @@ Deprecated or removed
have been deprecated in favor of `spdiagm(d => x)` and `spdiagm(d[1] => x[1], d[2] => x[2], ...)`
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]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :).

Sacha0
Sacha0 reviewed Dec 12, 2017
View reviewed changes
base/deprecated.jl Outdated
@@ -1861,6 +1861,12 @@ end
# PR #25030
@eval LinAlg @deprecate fillslots! fillstored! false

# 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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here, perhaps simplify the suggested replacement as much as possible?

Sacha0
Sacha0 reviewed Dec 12, 2017
View reviewed changes
base/sparse/sparsematrix.jl Outdated
"""
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fill!(view(S.nzval, 1:(S.colptr[S.n + 1] - 1)), x)?

Sacha0
Sacha0 reviewed Dec 12, 2017
View reviewed changes
base/sparse/sparse.jl Outdated
@@ -31,7 +31,7 @@ import Base: @get!, acos, acosd, acot, acotd, acsch, asech, asin, asind, asinh,

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!,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why export fillstored! from SparseArrays while LinAlg owns fillstored!?

Sacha0
Sacha0 reviewed Dec 12, 2017
View reviewed changes
base/sparse/sparsevector.jl Outdated
@@ -99,8 +99,7 @@ spzeros(len::Integer) = spzeros(Float64, len)
spzeros(::Type{T}, len::Integer) where {T} = SparseVector(len, Int[], T[])
spzeros(::Type{Tv}, ::Type{Ti}, len::Integer) where {Tv,Ti<:Integer} = SparseVector(len, Ti[], Tv[])

# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps nix the unnecessary type parameter?

Sacha0
Sacha0 reviewed Dec 12, 2017
View reviewed changes
test/sparse/sparse.jl Outdated
@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)
Copy link
Member

@Sacha0 Sacha0 Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the SparseArrays qualification when the definitions in SparseArrays extend the name from LinAlg? (Likewise below :).)

@Sacha0
Copy link
Member

Sacha0 commented Dec 12, 2017

Much thanks for clearing this one @fredrikekre! :)

@fredrikekre fredrikekre force-pushed the fe/spoons branch 3 times, most recently from bb8f554 to 9ec0111 Compare December 15, 2017 07:41
@fredrikekre
Copy link
Member Author

Rebased, planning to merge this when ci is done.

rfourquet
rfourquet reviewed Dec 15, 2017
View reviewed changes
base/deprecated.jl Outdated
@@ -1898,6 +1898,12 @@ end
# PR #25030
@eval LinAlg @deprecate fillslots! fillstored! false

# PR #TBD
Copy link
Member

@rfourquet rfourquet Dec 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo! the CI jobs to seem to not have started yet, so it's still time to update this without having CI resources wasted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future maybe CI will check for #TBD #TODO, [#CATS] etc!

@StefanKarpinski
Copy link
Member

I may have broken the build right when this was pushed, so probably worth fixing and rebasing.

@StefanKarpinski
Copy link
Member

Nevermind, I fixed it and pushed it for you.

@StefanKarpinski
Copy link
Member

I think we did the same thing at the same time...

@fredrikekre
Copy link
Member Author

I'm so confused... I only fixed the missing PR number that Rafael pointed out :)

@StefanKarpinski
Copy link
Member

Ok, I did that plus I rebased on top of a newer master where I fixed the merge mistake.

@Sacha0
Copy link
Member

Sacha0 commented Dec 15, 2017 •
edited
Loading

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
Copy link
Member

Sacha0 commented Dec 15, 2017 •
edited
Loading

The AV i686 failure is #25096, fixed by #25103. FreeBSD hit a timeout, seemingly unrelated. Best!

Sacha0
Sacha0 approved these changes Dec 15, 2017
View reviewed changes
Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm for the most recent version! Thanks @fredrikekre! :)

@fredrikekre fredrikekre merged commit 7cfcca2 into JuliaLang:master Dec 15, 2017
@fredrikekre fredrikekre deleted the fe/spoons branch December 15, 2017 18:56
fredrikekre added a commit to fredrikekre/julia that referenced this pull request Dec 29, 2017
@fredrikekre
fix deprecation of spones from JuliaLang#25037
5e2fe57
fredrikekre added a commit that referenced this pull request Dec 31, 2017
@fredrikekre
fix deprecation of spones from #25037 (#25330)
9018379
@cormullion cormullion mentioned this pull request Mar 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfourquet rfourquet rfourquet left review comments

@Sacha0 Sacha0 Sacha0 approved these changes

Assignees
No one assigned
Labels
deprecation This change introduces or involves a deprecation sparse Sparse arrays
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

spones documentation is incorrect
4 participants
@fredrikekre @Sacha0 @StefanKarpinski @rfourquet