rework of weighted graphs API, fixes #6 and Graphs#42 (#14)

I removed all methods using internal fields from the abstract functions.
I fixed the issue #6 occurring when removing an edge. It now delete the
coefficient (set the coefficient to a structural zero), instead of just
zeroing it. It is done more efficiently than by calling dropzeros! and
is as efficient as adding a new edge (outside for modifying a weight).
Now, the non edges are always structural zeros, so technically, we could
allow setting zero weight edges. I did not permit that as it would
probably be breaking, and because we can't add zeros from the weights
matrix constructor, as zeros are mapped to structural zeros.

Review needed please.

---------

Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
Co-authored-by: Simon Schölly <sischoel@gmail.com>

Author: 3 people

Project.toml

@@ -1,6 +1,6 @@
 name = "SimpleWeightedGraphs"
 uuid = "47aef6b3-ad0c-573a-a1e2-d07658019622"
-version = "1.2.2"
+version = "1.3.0"
 
 [deps]
 Graphs = "86223c79-3864-5bf0-83f7-82e725a168b6"

src/SimpleWeightedGraphs.jl

@@ -59,14 +59,11 @@ convert(::Type{SparseMatrixCSC{T, U}}, g::AbstractSimpleWeightedGraph) where T<:
 
 ### INTERFACE
 
-nv(g::AbstractSimpleWeightedGraph{T, U}) where T where U = T(size(g.weights, 1))
+nv(g::AbstractSimpleWeightedGraph{T, U}) where T where U = T(size(weights(g), 1))
 vertices(g::AbstractSimpleWeightedGraph{T, U}) where T where U = one(T):nv(g)
 eltype(x::AbstractSimpleWeightedGraph{T, U}) where T where U = T
 weighttype(x::AbstractSimpleWeightedGraph{T, U}) where T where U = U
 
-has_edge(g::AbstractSimpleWeightedGraph{T, U}, e::AbstractSimpleWeightedEdge) where T where U =
-    g.weights[dst(e), src(e)] != zero(U)
-
 # handles single-argument edge constructors such as pairs and tuples
 has_edge(g::AbstractSimpleWeightedGraph{T, U}, x) where T where U = has_edge(g, edgetype(g)(x))
 add_edge!(g::AbstractSimpleWeightedGraph{T, U}, x) where T where U = add_edge!(g, edgetype(g)(x))
@@ -88,42 +85,14 @@ function rem_edge!(g::AbstractSimpleWeightedGraph{T, U}, u::Integer, v::Integer)
     rem_edge!(g, edgetype(g)(T(u), T(v), one(U)))
 end
 
-@doc_str """
-    rem_vertex!(g::AbstractSimpleWeightedGraph, v)
-
-Remove the vertex `v` from graph `g`. Return false if removal fails
-(e.g., if vertex is not in the graph); true otherwise.
-
-### Implementation Notes
-This operation has to be performed carefully if one keeps external
-data structures indexed by edges or vertices in the graph, since
-internally the removal results in all vertices with indices greater than `v`
-being shifted down one.
-"""
-function rem_vertex!(g::AbstractSimpleWeightedGraph, v::Integer)
-    v in vertices(g) || return false
-    n = nv(g)
-
-    newweights = g.weights[1:nv(g) .!= v, :]
-    newweights = newweights[:, 1:nv(g) .!= v]
-
-    g.weights = newweights
-    return true
-end
-
-function outneighbors(g::AbstractSimpleWeightedGraph, v::Integer)
-    mat = g.weights
-    return view(mat.rowval, mat.colptr[v]:mat.colptr[v+1]-1)
-end
-
-get_weight(g::AbstractSimpleWeightedGraph, u::Integer, v::Integer) = g.weights[v, u]
+get_weight(g::AbstractSimpleWeightedGraph, u::Integer, v::Integer) = weights(g)[v, u]
 
 zero(g::T) where T<:AbstractSimpleWeightedGraph = T()
 
 # TODO: manipulte SparseMatrixCSC directly
 add_vertex!(g::AbstractSimpleWeightedGraph) = add_vertices!(g, 1)
 
-copy(g::T) where T <: AbstractSimpleWeightedGraph =  T(copy(g.weights))
+copy(g::T) where T <: AbstractSimpleWeightedGraph =  T(copy(weights(g)))
 
 
 const SimpleWeightedGraphEdge = SimpleWeightedEdge
@@ -136,6 +105,21 @@ include("persistence.jl")
 const WGraph = SimpleWeightedGraph
 const WDiGraph = SimpleWeightedDiGraph
 
+
+# return the index in nzval of mat[i, j]
+# we assume bounds are already checked
+# see https://github.com/JuliaSparse/SparseArrays.jl/blob/fa547689947fadd6c2f3d09ddfcb5f26536f18c8/src/sparsematrix.jl#L2492 for implementation
+@inbounds function _get_nz_index!(mat::SparseMatrixCSC, i::Integer, j::Integer)
+    # r1 and r2 are start and end of the column
+    r1 = Int(mat.colptr[j])
+    r2 = Int(mat.colptr[j+1]-1)
+    (r1 > r2) && return 0 # column is empty so we have a non structural zero
+    # search if i correspond to a stored value
+    indx = searchsortedfirst(mat.rowval, i, r1, r2, Base.Forward)
+    ((indx > r2) || (mat.rowval[indx] != i)) && return 0
+    return indx
+end
+
 SimpleWeightedDiGraph(g::SimpleWeightedGraph) = SimpleWeightedDiGraph(copy(g.weights))
 function SimpleWeightedDiGraph{T, U}(g::SimpleWeightedGraph) where {T<:Integer, U<:Real}
     return SimpleWeightedDiGraph(SparseMatrixCSC{U, T}(copy(g.weights)))

src/simpleweighteddigraph.jl

@@ -40,6 +40,14 @@ SimpleWeightedDiGraph{T,U}(g::SimpleWeightedDiGraph) where {T <: Integer, U <: R
 
 ne(g::SimpleWeightedDiGraph) = nnz(g.weights)
 
+function has_edge(g::SimpleWeightedDiGraph, u::Integer, v::Integer)
+    (u ∈ vertices(g) && v ∈ vertices(g)) || return false
+    _get_nz_index!(g.weights, v, u) != 0 # faster than Base.isstored
+end
+
+has_edge(g::SimpleWeightedDiGraph, e::AbstractEdge) =
+    has_edge(g, src(e), dst(e))
+
 function SimpleWeightedDiGraph{T,U}(n::Integer = 0) where {T<:Integer, U<:Real}
     weights = spzeros(U, T, T(n), T(n))
     return SimpleWeightedDiGraph{T, U}(weights)
@@ -73,7 +81,7 @@ function SimpleWeightedDiGraph(g::Graphs.AbstractGraph{T}, x::U) where {U <: Rea
 end
 
 # DiGraph(srcs, dsts, weights)
-function SimpleWeightedDiGraph(i::AbstractVector{T}, j::AbstractVector{T}, v::AbstractVector{U}; combine = +) where T<:Integer where U<:Real
+function SimpleWeightedDiGraph(i::AbstractVector{T}, j::AbstractVector{T}, v::AbstractVector{U}; combine = +) where {T<:Integer, U<:Real}
     m = max(maximum(j), maximum(i))
     SimpleWeightedDiGraph{T, U}(sparse(j, i, v, m, m, combine), permute=false)
 end
@@ -85,6 +93,10 @@ edgetype(::SimpleWeightedDiGraph{T, U}) where T<:Integer where U<:Real = SimpleW
 edges(g::SimpleWeightedDiGraph) = (SimpleWeightedEdge(x[2], x[1], x[3]) for x in zip(findnz(g.weights)...))
 weights(g::SimpleWeightedDiGraph) = g.weights'
 
+function outneighbors(g::SimpleWeightedDiGraph, v::Integer)
+    mat = g.weights
+    return view(mat.rowval, mat.colptr[v]:(mat.colptr[v+1]-1))
+end
 inneighbors(g::SimpleWeightedDiGraph, v::Integer) = g.weights[v,:].nzind
 
 # add_edge! will overwrite weights.
@@ -105,13 +117,41 @@ function add_edge!(g::SimpleWeightedDiGraph, e::SimpleWeightedGraphEdge)
     return true
 end
 
-function rem_edge!(g::SimpleWeightedDiGraph, e::SimpleWeightedGraphEdge)
-    has_edge(g, e) || return false
-    U = weighttype(g)
-    @inbounds g.weights[dst(e), src(e)] = zero(U)
+rem_edge!(g::SimpleWeightedDiGraph, e::AbstractEdge) =
+    rem_edge!(g, src(e), dst(e))
+
+function rem_edge!(g::SimpleWeightedDiGraph{T}, u::Integer, v::Integer) where {T}
+    (u ∈ vertices(g) && v ∈ vertices(g)) || return false
+    w = g.weights
+    indx = _get_nz_index!(w, v, u) # get the index in nzval
+    indx == 0 && return false # the edge does not exist
+    @view(w.colptr[(u+one(u)):end]) .-= T(1) # there is one value less in column u
+    # we remove the stored value
+    deleteat!(w.rowval, indx)
+    deleteat!(w.nzval, indx)
+
     return true
 end
 
+@doc_str """
+    rem_vertex!(g::SimpleWeightedDiGraph, v)
+
+Remove the vertex `v` from graph `g`. Return false if removal fails
+(e.g., if vertex is not in the graph); true otherwise.
+
+### Implementation Notes
+This operation has to be performed carefully if one keeps external
+data structures indexed by edges or vertices in the graph, since
+internally the removal results in all vertices with indices greater than `v`
+being shifted down one.
+"""
+function rem_vertex!(g::SimpleWeightedDiGraph, v::Integer)
+    v in vertices(g) || return false
+    n = nv(g)
+    all_except_v = (1:n) .!= v
+    g.weights = g.weights[all_except_v, all_except_v]
+    return true
+end
 
 copy(g::SimpleWeightedDiGraph) =  SimpleWeightedDiGraph(copy(g.weights'))
 
@@ -124,7 +164,7 @@ is_directed(::Type{<:SimpleWeightedDiGraph}) = true
 
 Equivalent to g[src(e), dst(e)].
 """
-function Base.getindex(g::SimpleWeightedDiGraph{T, U}, e::AbstractEdge, ::Val{:weight}) where {T, U}
+function Base.getindex(g::SimpleWeightedDiGraph, e::AbstractEdge, ::Val{:weight})
     return g.weights[dst(e), src(e)]
 end
 
@@ -133,6 +173,6 @@ end
 
 Return the weight of edge (i, j).
 """
-function Base.getindex(g::SimpleWeightedDiGraph{T, U}, i::Integer, j::Integer, ::Val{:weight}) where {T, U}
+function Base.getindex(g::SimpleWeightedDiGraph, i::Integer, j::Integer, ::Val{:weight})
     return g.weights[j, i]
 end

src/simpleweightedgraph.jl

@@ -20,6 +20,14 @@ end
 
 ne(g::SimpleWeightedGraph) = (nnz(g.weights) + nselfloop(g)) ÷ 2
 
+function has_edge(g::SimpleWeightedGraph, u::Integer, v::Integer)
+    u ∈ vertices(g) && v ∈ vertices(g) || return false
+    _get_nz_index!(g.weights, v, u) != 0 # faster than Base.isstored
+end
+
+has_edge(g::SimpleWeightedGraph, e::AbstractEdge) =
+    has_edge(g, src(e), dst(e))
+
 function nselfloop(g::SimpleWeightedGraph)
     n = 0
     for i in axes(g.weights, 1)
@@ -111,6 +119,11 @@ edgetype(::SimpleWeightedGraph{T, U}) where {T<:Integer, U<:Real} = SimpleWeight
 
 edges(g::SimpleWeightedGraph) = (SimpleWeightedEdge(x[1], x[2], x[3]) for x in zip(findnz(triu(g.weights))...))
 weights(g::SimpleWeightedGraph) = g.weights
+
+function outneighbors(g::SimpleWeightedGraph, v::Integer)
+    mat = g.weights
+    return view(mat.rowval, mat.colptr[v]:mat.colptr[v+1]-1)
+end
 inneighbors(g::SimpleWeightedGraph, x...) = outneighbors(g, x...)
 
 # add_edge! will overwrite weights.
@@ -132,11 +145,44 @@ function add_edge!(g::SimpleWeightedGraph, e::SimpleWeightedGraphEdge)
     return true
 end
 
-function rem_edge!(g::AbstractSimpleWeightedGraph, e::SimpleWeightedGraphEdge)
-    has_edge(g, e) || return false
-    U = weighttype(g)
-    @inbounds g.weights[dst(e), src(e)] = zero(U)
-    @inbounds g.weights[src(e), dst(e)] = zero(U)
+rem_edge!(g::SimpleWeightedGraph, e::AbstractEdge) =
+    rem_edge!(g, src(e), dst(e))
+
+function rem_edge!(g::SimpleWeightedGraph{T, U}, u::Integer, v::Integer) where {T<:Integer, U<:Real}
+    (u ∈ vertices(g) && v ∈ vertices(g)) || return false
+    w = g.weights
+    indx_uv = _get_nz_index!(w, u, v) # get the index in nzval
+    indx_uv == 0 && return false # the edge does not exist
+    @view(w.colptr[(v+one(v)):end]) .-= T(1) # there is one value less in column v
+    # we remove the stored value
+    deleteat!(w.rowval, indx_uv)
+    deleteat!(w.nzval, indx_uv)
+    (u == v) && return true
+    # same for the reverse edge
+    indx_vu = _get_nz_index!(w, v, u)
+    @view(w.colptr[(u+one(u)):end]) .-= T(1)
+    deleteat!(w.rowval, indx_vu)
+    deleteat!(w.nzval, indx_vu)
+    return true
+end
+
+@doc_str """
+    rem_vertex!(g::SimpleWeightedGraph, v)
+
+Remove the vertex `v` from graph `g`. Return false if removal fails
+(e.g., if vertex is not in the graph); true otherwise.
+
+### Implementation Notes
+This operation has to be performed carefully if one keeps external
+data structures indexed by edges or vertices in the graph, since
+internally the removal results in all vertices with indices greater than `v`
+being shifted down one.
+"""
+function rem_vertex!(g::SimpleWeightedGraph, v::Integer)
+    v in vertices(g) || return false
+    n = nv(g)
+    all_except_v = (1:n) .!= v
+    g.weights = g.weights[all_except_v, all_except_v]
     return true
 end
 
@@ -145,10 +191,11 @@ end
 
 is_directed(::Type{<:SimpleWeightedGraph}) = false
 
-function Base.getindex(g::SimpleWeightedGraph{T, U}, e::AbstractEdge, ::Val{:weight}) where {T, U}
+
+function Base.getindex(g::SimpleWeightedGraph, e::AbstractEdge, ::Val{:weight})
     return g.weights[src(e), dst(e)]
 end
 
-function Base.getindex(g::SimpleWeightedGraph{T, U}, i::Integer, j::Integer, ::Val{:weight}) where {T, U}
+function Base.getindex(g::SimpleWeightedGraph, i::Integer, j::Integer, ::Val{:weight})
     return g.weights[i, j]
 end

test/simpleweightedgraph.jl

@@ -74,6 +74,7 @@ using SimpleWeightedGraphs
         @test @inferred(ne(g)) == 3
 
         @test @inferred(rem_edge!(gc, 1, 2)) && @inferred(!has_edge(gc, 1, 2))
+        @test @inferred(inneighbors(gc, 2)) == @inferred(outneighbors(gc, 2)) == @inferred(neighbors(gc,2)) == [3]
         ga = @inferred(copy(g))
         @test @inferred(rem_vertex!(ga, 2)) && ne(ga) == 1
         @test @inferred(!rem_vertex!(ga, 10))
@@ -142,6 +143,7 @@ using SimpleWeightedGraphs
 
         @test @inferred(!rem_edge!(gc, 2, 1))
         @test @inferred(rem_edge!(gc, 1, 2)) && @inferred(!has_edge(gc, 1, 2))
+        @test @inferred(outneighbors(gc, 1)) == @inferred(neighbors(gc, 1)) == Int[]
         ga = @inferred(copy(g))
         @test @inferred(rem_vertex!(ga, 2)) && ne(ga) == 1
         @test @inferred(!rem_vertex!(ga, 10))