fix #69 and #70 (#71)

* fix #69 and #70

* v0.7.0 because breaking change

* arrange: no more codes as arguments

* more testing

* add_edge! modifies edge data if edge already present

* Update src/graphs.jl

---------

Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>

Project.toml

@@ -1,6 +1,6 @@
 name = "MetaGraphsNext"
 uuid = "fa8bd995-216d-47f1-8a91-f3b68fbeb377"
-version = "0.6.0"
+version = "0.7.0"
 
 [deps]
 Graphs = "86223c79-3864-5bf0-83f7-82e725a168b6"

src/dict_utils.jl

@@ -114,7 +114,7 @@ function _copy_props!(old_meta_graph::MetaGraph, new_meta_graph::MetaGraph, code
         code_1, code_2 = Tuple(new_edge)
         label_1 = vertex_labels[code_1]
         label_2 = vertex_labels[code_2]
-        new_meta_graph.edge_data[arrange(new_meta_graph, label_1, label_2, code_1, code_2)] = old_meta_graph.edge_data[arrange(
+        new_meta_graph.edge_data[arrange(new_meta_graph, label_1, label_2)] = old_meta_graph.edge_data[arrange(
             old_meta_graph, label_1, label_2
         )]
     end

src/directedness.jl

@@ -12,33 +12,19 @@ end
     arrange(graph, label_1, label_2)
 
 Sort two vertex labels in a default order (useful to uniquely express undirected edges).
+For undirected graphs, the default order is based on the labels themselves
+to be robust to vertex re-coding, so the labels need to support `<`.
 """
 function arrange end
 
-@traitfn function arrange(
-    ::MG, label_1, label_2, _drop...
-) where {MG <: MetaGraph; IsDirected{MG}}
+@traitfn function arrange(::MG, label_1, label_2) where {MG <: MetaGraph; IsDirected{MG}}
     return label_1, label_2
 end
 
-@traitfn function arrange(
-    ::MG, label_1, label_2, code_1, code_2
-) where {MG <: MetaGraph; !IsDirected{MG}}
-    if code_1 < code_2
+@traitfn function arrange(::MG, label_1, label_2) where {MG <: MetaGraph; !IsDirected{MG}}
+    if label_1 < label_2
         (label_1, label_2)
     else
         (label_2, label_1)
     end
 end
-
-@traitfn function arrange(
-    meta_graph::MG, label_1, label_2
-) where {MG <: MetaGraph; !IsDirected{MG}}
-    return arrange(
-        meta_graph,
-        label_1,
-        label_2,
-        code_for(meta_graph, label_1),
-        code_for(meta_graph, label_2),
-    )
-end

src/graphs.jl

@@ -174,14 +174,18 @@ Add an edge `(label_1, label_2)` to MetaGraph `meta_graph` with metadata `data`.
 If the `EdgeData` type of `meta_graph` is `Nothing`, `data` can be omitted.
 
 Return `true` if the edge has been added, `false` otherwise.
+If `(label_1, label_2)` already existed, its data is updated to `data` and `false` is returned nonetheless.
 """
 function Graphs.add_edge!(meta_graph::MetaGraph, label_1, label_2, data)
     if !haskey(meta_graph, label_1) || !haskey(meta_graph, label_2)
         return false
     end
     code_1, code_2 = code_for(meta_graph, label_1), code_for(meta_graph, label_2)
-    label_tup = arrange(meta_graph, label_1, label_2, code_1, code_2)
+    label_tup = arrange(meta_graph, label_1, label_2)
     meta_graph.edge_data[label_tup] = data
+    if has_edge(meta_graph.graph, code_1, code_2)
+        return false
+    end
     ne_prev = ne(meta_graph.graph)
     add_edge!(meta_graph.graph, code_1, code_2)
     if ne(meta_graph.graph) == ne_prev  # undo
@@ -206,16 +210,10 @@ function _rem_vertex!(meta_graph::MetaGraph, label, code)
     edge_data = meta_graph.edge_data
     last_vertex_code = nv(meta_graph)
     for out_neighbor in outneighbors(meta_graph, code)
-        delete!(
-            edge_data,
-            arrange(meta_graph, label, vertex_labels[out_neighbor], code, out_neighbor),
-        )
+        delete!(edge_data, arrange(meta_graph, label, vertex_labels[out_neighbor]))
     end
     for in_neighbor in inneighbors(meta_graph, code)
-        delete!(
-            edge_data,
-            arrange(meta_graph, vertex_labels[in_neighbor], label, in_neighbor, code),
-        )
+        delete!(edge_data, arrange(meta_graph, vertex_labels[in_neighbor], label))
     end
     removed = rem_vertex!(meta_graph.graph, code)
     if removed
@@ -244,9 +242,9 @@ end
 
 function Graphs.rem_edge!(meta_graph::MetaGraph, code_1::Integer, code_2::Integer)
     removed = rem_edge!(meta_graph.graph, code_1, code_2)
-    if removed
+    if removed # assume that vertex codes were not modified by edge removal
         label_1, label_2 = label_for(meta_graph, code_1), label_for(meta_graph, code_2)
-        delete!(meta_graph.edge_data, arrange(meta_graph, label_1, label_2, code_1, code_2))
+        delete!(meta_graph.edge_data, arrange(meta_graph, label_1, label_2))
     end
     return removed
 end

src/weights.jl

@@ -65,7 +65,7 @@ function Base.getindex(meta_weights::MetaWeights, code_1::Integer, code_2::Integ
         labels = meta_graph.vertex_labels
         weight_function = get_weight_function(meta_graph)
         arranged_label_1, arranged_label_2 = arrange(
-            meta_graph, labels[code_1], labels[code_2], code_1, code_2
+            meta_graph, labels[code_1], labels[code_2]
         )
         return weight_function(meta_graph[arranged_label_1, arranged_label_2])
     else

test/misc.jl

@@ -8,6 +8,11 @@ function test_labels_codes(mg::MetaGraph)
     for (label_1, label_2) in edge_labels(mg)
         @test has_edge(mg, code_for(mg, label_1), code_for(mg, label_2))
     end
+    # below: arrange(edges) ⊆ keys of mg.edge_data. then = because same length
+    for e in edge_labels(mg)
+        @test_logs mg[e...]  # no log, no error
+    end
+    @test length(keys(mg.edge_data)) == ne(mg)
     for label_1 in labels(mg)
         for label_2 in outneighbor_labels(mg, label_1)
             @test has_edge(mg, code_for(mg, label_1), code_for(mg, label_2))
@@ -24,17 +29,24 @@ end
         :red => (255, 0, 0), :green => (0, 255, 0), :blue => (0, 0, 255)
     ]
     edges_description = [
-        (:red, :green) => :yellow, (:red, :blue) => :magenta, (:green, :blue) => :cyan
+        (:green, :red) => :yellow, (:blue, :red) => :magenta, (:blue, :green) => :cyan
     ]
 
     colors = MetaGraph(graph, vertices_description, edges_description, "additive colors")
     test_labels_codes(colors)
 
+    # attempt to add an existing edge: non-standard order, different data
+    @test !add_edge!(colors, :green, :blue, :teal)
+    @test length(colors.edge_data) == ne(colors)
+    @test colors[:blue, :green] == :teal
+
     # Delete vertex in a copy and test again
 
     colors_copy = copy(colors)
     rem_vertex!(colors_copy, 1)
-    test_labels_codes(colors)
+    test_labels_codes(colors_copy)
+    @test ne(colors_copy) == 1
+    @test colors_copy[:blue, :green] == :teal
 end
 
 @testset verbose = true "Short-form add_vertex!/add_edge!" begin
@@ -45,6 +57,7 @@ end
     @test add_vertex!(mg, :A)
     @test add_vertex!(mg, :B)
     @test add_edge!(mg, :A, :B)
+    @test !add_edge!(mg, :A, :C)
 
     # long-form
     mg2 = MetaGraph(

test/tutorial/1_basics.jl

@@ -16,7 +16,7 @@ colors = MetaGraph(
     graph_data="additive colors",  # tag for the whole graph
 )
 
-# The `label_type` argument defines how vertices will be referred to, it can be anything you want (although integer types are generally discouraged, to avoid confusion with the vertex codes used by Graphs.jl). The `vertex_data_type` and `edge_data_type` type determine what kind of data will be associated with each vertex and edge. Finally, `graph_data` can contain an arbitrary object associated with the graph as a whole.
+# The `label_type` argument defines how vertices will be referred to. It can be anything you want, provided that pairs of labels can be compared with `<`. Integer types are generally discouraged, to avoid confusion with the vertex codes used by Graphs.jl. The `vertex_data_type` and `edge_data_type` type determine what kind of data will be associated with each vertex and edge. Finally, `graph_data` can contain an arbitrary object associated with the graph as a whole.
 
 # If you don't care about labels at all, using the integer vertex codes as labels may be reasonable. Just keep in mind that labels do not change with vertex deletion, whereas vertex codes get decreased, so the coherence will be broken.
 

test/tutorial/2_graphs.jl

@@ -35,7 +35,8 @@ cities[:Paris, :Berlin] = 878;
 is_directed(cities)
 @test @inferred !is_directed(cities)  #src
 @test !istrait(IsDirected{typeof(cities)})  #src
-@test MetaGraphsNext.arrange(cities, :London, :Paris) == (:Paris, :London)  #src
+@test MetaGraphsNext.arrange(cities, :London, :Paris) == (:London, :Paris)  #src
+@test MetaGraphsNext.arrange(cities, :Paris, :London) == (:London, :Paris)  #src
 #-
 eltype(cities)
 @test @inferred eltype(cities) == Int  #src