sbromberger / LightGraphs.jl

An optimized graphs package for the Julia programming language
Other
670 stars 184 forks source link

[BUG] `adjacency_matrix` fails for `SimpleGraph` with self-loops (Julia 1.7) #1594

Open mtfishman opened 3 years ago

mtfishman commented 3 years ago

Description of bug adjacency_matrix fails for SimpleGraph with self-loops in Julia 1.7.

How to reproduce

julia> versioninfo()
Julia Version 1.7.0-rc1
Commit 9eade6195e (2021-09-12 06:45 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Xeon(R) E-2176M  CPU @ 2.70GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-12.0.1 (ORCJIT, skylake)
Environment:
  JULIA_EDITOR = vim

(@v1.7) pkg> st LightGraphs
      Status `~/.julia/environments/v1.7/Project.toml`
  [093fc24a] LightGraphs v1.3.5

julia> using LightGraphs

julia> g = Graph(Edge.([1 => 2, 2 => 3]))
{3, 2} undirected simple Int64 graph

julia> adjacency_matrix(g)
3×3 SparseArrays.SparseMatrixCSC{Int64, Int64} with 4 stored entries:
 ⋅  1  ⋅
 1  ⋅  1
 ⋅  1  ⋅

julia> add_edge!(g, 1 => 1)
true

julia> adjacency_matrix(g)
ERROR: ArgumentError: Illegal buffers for SparseMatrixCSC construction 3 [1, 3, 5, 6] [1, 2, 1, 3, 2] [1, 1, 1, 1, 1, 1]
Stacktrace:
 [1] SparseMatrixCSC
   @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.7/SparseArrays/src/sparsematrix.jl:29 [inlined]
 [2] SparseArrays.SparseMatrixCSC(m::Int64, n::Int64, colptr::Vector{Int64}, rowval::Vector{Int64}, nzval::Vector{Int64})
   @ SparseArrays /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.7/SparseArrays/src/sparsematrix.jl:44
 [3] _adjacency_matrix(g::SimpleGraph{Int64}, T::DataType, neighborfn::typeof(inneighbors), nzmult::Int64)
   @ LightGraphs.LinAlg ~/.julia/packages/LightGraphs/IgJif/src/linalg/spectral.jl:63
 [4] #adjacency_matrix#2
   @ ~/.julia/packages/LightGraphs/IgJif/src/linalg/spectral.jl:25 [inlined]
 [5] adjacency_matrix (repeats 2 times)
   @ ~/.julia/packages/LightGraphs/IgJif/src/linalg/spectral.jl:20 [inlined]
 [6] top-level scope
   @ REPL[35]:1

It seems to work fine for directed graphs:

julia> g = DiGraph(Edge.([1 => 2, 2 => 3]))
{3, 2} directed simple Int64 graph

julia> adjacency_matrix(g)
3×3 SparseArrays.SparseMatrixCSC{Int64, Int64} with 2 stored entries:
 ⋅  1  ⋅
 ⋅  ⋅  1
 ⋅  ⋅  ⋅

julia> add_edge!(g, 1 => 1)
true

julia> adjacency_matrix(g)
3×3 SparseArrays.SparseMatrixCSC{Int64, Int64} with 3 stored entries:
 1  1  ⋅
 ⋅  ⋅  1
 ⋅  ⋅  ⋅
mtfishman commented 3 years ago

Related to https://github.com/JuliaGraphs/NetworkLayout.jl/issues/41.

mtfishman commented 3 years ago

Also works fine with Julia 1.6:

julia> versioninfo()
Julia Version 1.6.2
Commit 1b93d53fc4 (2021-07-14 15:36 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Xeon(R) E-2176M  CPU @ 2.70GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, skylake)
Environment:
  JULIA_EDITOR = vim

(@v1.6) pkg> st LightGraphs
      Status `~/.julia/environments/v1.6/Project.toml`
  [093fc24a] LightGraphs v1.3.5

julia> using LightGraphs

julia> g = Graph(Edge.([1 => 2, 2 => 3]))
{3, 2} undirected simple Int64 graph

julia> adjacency_matrix(g)
3×3 SparseArrays.SparseMatrixCSC{Int64, Int64} with 4 stored entries:
 ⋅  1  ⋅
 1  ⋅  1
 ⋅  1  ⋅

julia> add_edge!(g, 1 => 1)
true

julia> adjacency_matrix(g)
3×3 SparseArrays.SparseMatrixCSC{Int64, Int64} with 5 stored entries:
 2  1  ⋅
 1  ⋅  1
 ⋅  1  ⋅

so maybe an issue with SparseArrays?

mtfishman commented 3 years ago

It looks like the error in Julia 1.7 is caused by stricter checks in the SparseMatrixCSC constructor introduced in this PR: https://github.com/JuliaLang/julia/pull/40523

However, my guess is that it is more an issue on the LightGraphs side on how it is using the constructor here: https://github.com/JuliaGraphs/LightGraphs.jl/blob/b210395e8cbd109dc04c286f8af0c267cad47a85/src/linalg/spectral.jl#L53-L72 in the case of self-loops when the graph is undirected.

mtfishman commented 3 years ago

Adding a line like:

            if !(T <: Bool) && !is_directed(g)
                nz -= 1
            end

after line 57 seems to help. I think the issue is that the original definition of nz is double counting the self-loops as nonzero values in the sparse matrix, which is being caught in the updated SparseMatrixCSC constructor.