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.

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

deprecate diagm(A::SparseMatrixCSC) in favor of spdiagm(sparsevec(A)) #23341

Merged
merged 2 commits into from
Aug 22, 2017

Conversation

fredrikekre
Copy link
Member

To be consistent with

julia> diagm(rand(3,1))
ERROR: MethodError: no method matching diagm(::Array{Float64,2})
[...]

julia> diagm(rand(1,3))
ERROR: MethodError: no method matching diagm(::Array{Float64,2})
[...]

Step 1 in JuliaLang/LinearAlgebra.jl#457

@fredrikekre fredrikekre added linear algebra Linear algebra sparse Sparse arrays labels Aug 18, 2017
@fredrikekre fredrikekre requested a review from Sacha0 August 18, 2017 23:28
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 pending CI! :)

Edit: Perhaps add a NEWS.md entry?

@Sacha0 Sacha0 added needs news A NEWS entry is required for this change deprecation This change introduces or involves a deprecation labels Aug 19, 2017
@fredrikekre
Copy link
Member Author

Following the discussion in JuliaLang/LinearAlgebra.jl#457 it looks like we will not get rid of spdiagm any time soon, and no matter what, this should actually be a method of spdiagm, rather than diagm. Perhaps with a specialized spdiagm_internal

function spdiagm_internal(B, d)
ndiags = length(d)
if length(B) != ndiags; throw(ArgumentError("first argument should be a tuple of length(d)=$ndiags arrays of diagonals")); end
ncoeffs = 0
for vec in B
ncoeffs += length(vec)
end
I = Vector{Int}(ncoeffs)
J = Vector{Int}(ncoeffs)
V = Vector{promote_type(map(eltype, B)...)}(ncoeffs)
id = 0
i = 0
for vec in B
id += 1
diag = d[id]
numel = length(vec)
if diag < 0
row = -diag
col = 0
elseif diag > 0
row = 0
col = diag
else
row = 0
col = 0
end
range = 1+i:numel+i
I[range] = row+1:row+numel
J[range] = col+1:col+numel
copy!(view(V, range), vec)
i += numel
end
return (I,J,V)
end

for SparseVector. I will update the PR :)

@fredrikekre fredrikekre removed the needs news A NEWS entry is required for this change label Aug 20, 2017
@fredrikekre fredrikekre changed the title deprecate diagm(A::SparseMatrixCSC) in favor of diagm(sparsevec(A)) deprecate diagm(A::SparseMatrixCSC) in favor of spdiagm(sparsevec(A)) Aug 20, 2017
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@8f5f981). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   JuliaLang/julia#23341   +/-   ##
=========================================
  Coverage          ?   54.98%           
=========================================
  Files             ?      275           
  Lines             ?    51145           
  Branches          ?        0           
=========================================
  Hits              ?    28121           
  Misses            ?    23024           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f5f981...4e7da40. Read the comment docs.

@andreasnoack andreasnoack merged commit 1db7b8f into master Aug 22, 2017
@andreasnoack andreasnoack deleted the fe/sparse-diagm branch August 22, 2017 16:53
@tkelman
Copy link
Contributor

tkelman commented Aug 30, 2017

How does spdiagm compare in performance to the old specialized method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation linear algebra Linear algebra sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants