search

QuantumBFS / LuxurySparse.jl

A luxury sparse matrix package for Julia
https://quantumbfs.github.io/LuxurySparse.jl/latest/
Other
33 stars 7 forks source link

fix-diag to sparse conversion #21

Closed GiggleLiu closed 5 years ago

Roger-luo commented 5 years ago

CI is not pass.

Roger-luo commented 5 years ago

XRef: https://github.com/JuliaLang/julia/pull/32466 issue: https://github.com/JuliaLang/julia/issues/31770

I'll patch it here for now. But this PR should be in the coming release, so should be removed after 1.3 is out this year, or we would contain type piracy.

This issue is because the conversion was defined in sparse instead of SparseMatrixCSC, not just because the lack of constructors.

https://github.com/JuliaLang/julia/blob/55e36cc308b66d3472990a06b2797f9f9154ea0a/stdlib/SparseArrays/src/sparsematrix.jl#L778

I don't think it would appear in v1.2, but should be in v1.3 period, not sure if this would get backported, and I think you missed the type conversion of the diagonal part, it should be copied instead of view (Diagonal is not sub array, should use an explicit type for diagonal view to avoid heap allocation or use UnsafeArray).

function sparse(D::Diagonal{T}) where T
    m = length(D.diag)
    return SparseMatrixCSC(m, m, Vector(1:(m+1)), Vector(1:m), Vector{T}(D.diag))
end
Roger-luo commented 5 years ago

PS. I think this is one reason you want to get rid of this kind of auto conversion interface like sparse, the same as mat we should use explicit constructors more (if possible) in the future.

codecov[bot] commented 5 years ago

Codecov Report

Merging #21 into master will increase coverage by 0.11%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   70.86%   70.98%   +0.11%     
==========================================
  Files          12       12              
  Lines         484      486       +2     
==========================================
+ Hits          343      345       +2     
  Misses        141      141
Impacted Files Coverage Δ
src/conversions.jl 32.75% <100%> (+2.4%) :arrow_up:

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 ad75099...5969916. Read the comment docs.