From 57cd3ac7de62a664f7d273c3834940f630c812c8 Mon Sep 17 00:00:00 2001
From: Etienne dg <etienne.mace_de_gastines@insa-rouen.fr>
Date: Thu, 6 Jul 2023 10:48:18 +0200
Subject: [PATCH] fix is_cyclic and topological_sort (#284)

* fix quadratic time in is_cyclic and topological_sort

* Add comments

* renaming queue to stack

* fix topological_sort

* fix is_cyclic

* fix indentation

* fix indentation

* Update comment
---
 src/traversals/dfs.jl  | 33 +++++++++++++++++----------------
 test/traversals/dfs.jl | 18 ++++++++++++++++++
 2 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/src/traversals/dfs.jl b/src/traversals/dfs.jl
index 35dd1d54c..03eaca158 100644
--- a/src/traversals/dfs.jl
+++ b/src/traversals/dfs.jl
@@ -32,30 +32,30 @@ function is_cyclic end
 end
 # see https://github.com/mauro3/SimpleTraits.jl/issues/47#issuecomment-327880153 for syntax
 @traitfn function is_cyclic(g::AG::IsDirected) where {T,AG<:AbstractGraph{T}}
-    # 0 if not visited, 1 if visited, 2 if in the current dfs path, 3 if fully explored 
+    # 0 if not visited, 1 if in the current dfs path, 2 if fully explored
     vcolor = zeros(UInt8, nv(g))
     vertex_stack = Vector{T}()
     for v in vertices(g)
         vcolor[v] != 0 && continue
         push!(vertex_stack, v)
-        vcolor[v] = 1
         while !isempty(vertex_stack)
             u = vertex_stack[end]
-            if vcolor[u] == 1
-                vcolor[u] = 2
+            if vcolor[u] == 0
+                vcolor[u] = 1
                 for n in outneighbors(g, u)
                     # we hit a loop when reaching back a vertex of the main path
-                    if vcolor[n] == 2
+                    if vcolor[n] == 1
                         return true
                     elseif vcolor[n] == 0
                         # we store neighbors, but these are not yet on the path
-                        vcolor[n] = 1
                         push!(vertex_stack, n)
                     end
                 end
             else
-                vcolor[u] = 3
                 pop!(vertex_stack)
+                if vcolor[u] == 1 
+                    vcolor[u] = 2
+                end
             end
         end
     end
@@ -87,32 +87,33 @@ graph `g` as a vector of vertices in topological order.
 function topological_sort_by_dfs end
 # see https://github.com/mauro3/SimpleTraits.jl/issues/47#issuecomment-327880153 for syntax
 @traitfn function topological_sort_by_dfs(g::AG::IsDirected) where {T,AG<:AbstractGraph{T}}
-    # 0 if not visited, 1 if visited, 2 if in the current dfs path, 3 if fully explored 
+    # 0 if not visited, 1 if in the current dfs path, 2 if fully explored
     vcolor = zeros(UInt8, nv(g))
     verts = Vector{T}()
     vertex_stack = Vector{T}()
     for v in vertices(g)
         vcolor[v] != 0 && continue
         push!(vertex_stack, v)
-        vcolor[v] = 1
         while !isempty(vertex_stack)
             u = vertex_stack[end]
-            if vcolor[u] == 1
-                vcolor[u] = 2
+            if vcolor[u] == 0
+                vcolor[u] = 1
                 for n in outneighbors(g, u)
                     # we hit a loop when reaching back a vertex of the main path
-                    if vcolor[n] == 2
+                    if vcolor[n] == 1
                         error("The input graph contains at least one loop.") # TODO 0.7 should we use a different error?
                     elseif vcolor[n] == 0
                         # we store neighbors, but these are not yet on the path
-                        vcolor[n] = 1
                         push!(vertex_stack, n)
                     end
                 end
-            else
-                vcolor[u] = 3
+                else
                 pop!(vertex_stack)
-                pushfirst!(verts, u)
+                # if vcolor[u] == 2, the vertex was already explored and added to verts
+                if vcolor[u] == 1 
+                    vcolor[u] = 2
+                    pushfirst!(verts, u)
+                end
             end
         end
     end
diff --git a/test/traversals/dfs.jl b/test/traversals/dfs.jl
index 187b12fcb..a77f649b1 100644
--- a/test/traversals/dfs.jl
+++ b/test/traversals/dfs.jl
@@ -23,6 +23,15 @@
             0 0 0 1 0 0 0
         ],
     )
+    # non regression test following https://github.com/JuliaGraphs/Graphs.jl/pull/266#issuecomment-1621698039
+    g6 = SimpleDiGraph(4)
+    add_edge!(g6, 1, 2)
+    add_edge!(g6, 2, 3)
+    add_edge!(g6, 2, 4)
+    add_edge!(g6, 4, 3)
+    g7 = copy(g6)
+    add_edge!(g7, 3, 4)
+    
     @testset "dfs_tree" begin
         for g in testdigraphs(g5)
             z = @inferred(dfs_tree(GenericDiGraph(g), 1))
@@ -45,6 +54,11 @@
             @test @inferred(is_cyclic(g))
             @test_throws ErrorException topological_sort(g)
         end
+
+        # non regression test following https://github.com/JuliaGraphs/Graphs.jl/pull/266#issuecomment-1621698039
+        for g in testdigraphs(g6)
+            @test @inferred(topological_sort(g)) == [1, 2, 4, 3]
+        end
     end
 
     @testset "topological_sort_by_dfs" begin
@@ -84,5 +98,9 @@
         for g in testgraphs(gloop_undirected)
             @test @inferred(is_cyclic(g))
         end
+        # non regression test following https://github.com/JuliaGraphs/Graphs.jl/pull/266#issuecomment-1621698039
+        for g in testdigraphs(g7)
+            @test @inferred(is_cyclic(g))
+        end
     end
 end