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

Conversion from SparseMatrixCSC to Matrix uses the same eltype. #49

Closed
blegat opened this issue May 7, 2018 · 4 comments
Closed

Conversion from SparseMatrixCSC to Matrix uses the same eltype. #49

blegat opened this issue May 7, 2018 · 4 comments

Comments

@blegat
Copy link
Contributor

blegat commented May 7, 2018

When converting a SparseMatrixCSC to a Matrix, the current implementation assumes that it can store the result in a matrix of the same eltype

https://github.com/JuliaLang/julia/blob/989de7903e492aa25f5a888c19b4b8e8a3f9f1f0/stdlib/SparseArrays/src/sparsematrix.jl#L404

However, this does not work of zero of the eltype returns an element of a different type.

MCWE:

struct A end
Base.zero(::Type{A}) = 0
using SparseArrays
S = sparse([1, 2], [1, 2], [A(), A()])
Matrix(S)

output:

ERROR: LoadError: MethodError: Cannot `convert` an object of type Int64 to an object of type A
Closest candidates are:
  convert(::Type{T}, ::T) where T at essentials.jl:123
Stacktrace:
 [1] fill!(::Array{A,2}, ::Int64) at ./array.jl:220
 [2] zeros at ./array.jl:403 [inlined]
 [3] zeros at ./array.jl:400 [inlined]
 [4] Array{T,2} where T(::SparseMatrixCSC{A,Int64}) at /home/blegat/git/julia/usr/share/julia/stdlib/v0.7/SparseArrays/src/sparsematrix.jl:404
...

The error is due to the call zero(A, 2, 2).

This example is a bit artificial but the problem also occurs in JuMP with sparse matrices of VariableRef for which the zero type is AffExpr.

@martinholters
Copy link
Contributor

So the element type should be promote_type(Tv, typeof(zero(Tv)))?
From a brief look, the current code tries to avoid calling zero if the matrix is, in fact, dense. If we want to preserve that, we would need to preserve the element type Tv in the dense case, i.e.

A = length(S) == nnz(S) ? Matrix{Tv}(undef, S.m, S.n) : zeros(promote_type(Tv, typeof(zero(Tv))), S.m, S.n)

This would lead to a type-instability in the rare case that Tv != typeof(zero(Tv)). Not sure whether that's a problem.

@blegat
Copy link
Contributor Author

blegat commented May 7, 2018

So the element type should be promote_type(Tv, typeof(zero(Tv))) ?

Yes, that would work.

This would lead to a type-instability in the rare case that Tv != typeof(zero(Tv)). Not sure whether that's a problem.

In which case does one need to convert a full SparseMatrixCSC that has an eltype that does not implement zero into a Matrix ? Maybe we should just use zeros in all case.

@mbauman
Copy link
Contributor

mbauman commented May 7, 2018

There's a bigger problem with this premise: eltype(S) is wrong. It should be Union{Tv, typeof(zero(Tv))}. Worse: I don't think we can express that in the subtype relationship to AbstractArray.

@KristofferC KristofferC transferred this issue from JuliaLang/julia Jan 14, 2022
@SobhanMP
Copy link
Member

SobhanMP commented Aug 10, 2022

from the doc of zero

Get the additive identity element for the type of x

this is just wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants