From 80b7b57e682ee051573d7cc10a561d8b812b81d7 Mon Sep 17 00:00:00 2001 From: etienneINSA Date: Thu, 29 Jun 2023 00:42:42 +0200 Subject: [PATCH 1/8] fix quadratic time in is_cyclic and topological_sort --- src/traversals/dfs.jl | 54 +++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/src/traversals/dfs.jl b/src/traversals/dfs.jl index bb355250e..e272f1f8a 100644 --- a/src/traversals/dfs.jl +++ b/src/traversals/dfs.jl @@ -33,26 +33,25 @@ 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}} vcolor = zeros(UInt8, nv(g)) + S = Vector{T}() for v in vertices(g) vcolor[v] != 0 && continue - S = Vector{T}([v]) + push!(S, v) vcolor[v] = 1 while !isempty(S) u = S[end] - w = 0 - for n in outneighbors(g, u) - if vcolor[n] == 1 - return true - elseif vcolor[n] == 0 - w = n - break + if vcolor[u] == 1 + vcolor[u] = 2 + for n in outneighbors(g, u) + if vcolor[n] == 2 + return true + elseif vcolor[n] == 0 + vcolor[n] = 1 + push!(S, n) + end end - end - if w != 0 - vcolor[w] = 1 - push!(S, w) else - vcolor[u] = 2 + vcolor[u] = 3 pop!(S) end end @@ -87,32 +86,31 @@ function topological_sort_by_dfs end @traitfn function topological_sort_by_dfs(g::AG::IsDirected) where {T,AG<:AbstractGraph{T}} vcolor = zeros(UInt8, nv(g)) verts = Vector{T}() + S = Vector{T}() for v in vertices(g) vcolor[v] != 0 && continue - S = Vector{T}([v]) + push!(S, v) vcolor[v] = 1 while !isempty(S) u = S[end] - w = 0 - for n in outneighbors(g, u) - 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 - w = n - break + if vcolor[u] == 1 + vcolor[u] = 2 + for n in outneighbors(g, u) + if vcolor[n] == 2 + error("The input graph contains at least one loop.") # TODO 0.7 should we use a different error? + elseif vcolor[n] == 0 + vcolor[n] = 1 + push!(S, n) + end end - end - if w != 0 - vcolor[w] = 1 - push!(S, w) else - vcolor[u] = 2 - push!(verts, u) + vcolor[u] = 3 pop!(S) + pushfirst!(verts, u) end end end - return reverse(verts) + return verts end """ From a516250412f766bbf9fcd2632487a11f154636d0 Mon Sep 17 00:00:00 2001 From: Etienne dg Date: Thu, 29 Jun 2023 15:23:53 +0200 Subject: [PATCH 2/8] Add comments --- src/traversals/dfs.jl | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/traversals/dfs.jl b/src/traversals/dfs.jl index e272f1f8a..171a4f3ed 100644 --- a/src/traversals/dfs.jl +++ b/src/traversals/dfs.jl @@ -32,27 +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 vcolor = zeros(UInt8, nv(g)) - S = Vector{T}() + vertex_queue = Vector{T}() for v in vertices(g) vcolor[v] != 0 && continue - push!(S, v) + push!(vertex_queue, v) vcolor[v] = 1 - while !isempty(S) - u = S[end] + while !isempty(vertex_queue) + u = vertex_queue[end] if vcolor[u] == 1 vcolor[u] = 2 for n in outneighbors(g, u) + # we hit a loop when reaching back a vertex of the main path if vcolor[n] == 2 return true elseif vcolor[n] == 0 + # we store neighbors, but these are not yet on the path vcolor[n] = 1 - push!(S, n) + push!(vertex_queue, n) end end else vcolor[u] = 3 - pop!(S) + pop!(vertex_queue) end end end @@ -84,28 +87,31 @@ 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 vcolor = zeros(UInt8, nv(g)) verts = Vector{T}() - S = Vector{T}() + vertex_queue = Vector{T}() for v in vertices(g) vcolor[v] != 0 && continue - push!(S, v) + push!(vertex_queue, v) vcolor[v] = 1 - while !isempty(S) - u = S[end] + while !isempty(vertex_queue) + u = vertex_queue[end] if vcolor[u] == 1 vcolor[u] = 2 for n in outneighbors(g, u) + # we hit a loop when reaching back a vertex of the main path if vcolor[n] == 2 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!(S, n) + push!(vertex_queue, n) end end else vcolor[u] = 3 - pop!(S) + pop!(vertex_queue) pushfirst!(verts, u) end end From 2562fe0a5f505f7b590bda0dfc02fa972191030c Mon Sep 17 00:00:00 2001 From: Etienne dg Date: Mon, 3 Jul 2023 09:55:50 +0200 Subject: [PATCH 3/8] renaming queue to stack --- src/traversals/dfs.jl | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/traversals/dfs.jl b/src/traversals/dfs.jl index 171a4f3ed..35dd1d54c 100644 --- a/src/traversals/dfs.jl +++ b/src/traversals/dfs.jl @@ -34,13 +34,13 @@ end @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 vcolor = zeros(UInt8, nv(g)) - vertex_queue = Vector{T}() + vertex_stack = Vector{T}() for v in vertices(g) vcolor[v] != 0 && continue - push!(vertex_queue, v) + push!(vertex_stack, v) vcolor[v] = 1 - while !isempty(vertex_queue) - u = vertex_queue[end] + while !isempty(vertex_stack) + u = vertex_stack[end] if vcolor[u] == 1 vcolor[u] = 2 for n in outneighbors(g, u) @@ -50,12 +50,12 @@ end elseif vcolor[n] == 0 # we store neighbors, but these are not yet on the path vcolor[n] = 1 - push!(vertex_queue, n) + push!(vertex_stack, n) end end else vcolor[u] = 3 - pop!(vertex_queue) + pop!(vertex_stack) end end end @@ -90,13 +90,13 @@ function topological_sort_by_dfs end # 0 if not visited, 1 if visited, 2 if in the current dfs path, 3 if fully explored vcolor = zeros(UInt8, nv(g)) verts = Vector{T}() - vertex_queue = Vector{T}() + vertex_stack = Vector{T}() for v in vertices(g) vcolor[v] != 0 && continue - push!(vertex_queue, v) + push!(vertex_stack, v) vcolor[v] = 1 - while !isempty(vertex_queue) - u = vertex_queue[end] + while !isempty(vertex_stack) + u = vertex_stack[end] if vcolor[u] == 1 vcolor[u] = 2 for n in outneighbors(g, u) @@ -106,12 +106,12 @@ function topological_sort_by_dfs end elseif vcolor[n] == 0 # we store neighbors, but these are not yet on the path vcolor[n] = 1 - push!(vertex_queue, n) + push!(vertex_stack, n) end end else vcolor[u] = 3 - pop!(vertex_queue) + pop!(vertex_stack) pushfirst!(verts, u) end end From 6d0a2adc04fa84536f23f67f9825f009250be7dd Mon Sep 17 00:00:00 2001 From: etienneINSA Date: Wed, 5 Jul 2023 15:39:21 +0200 Subject: [PATCH 4/8] fix topological_sort --- src/traversals/dfs.jl | 61 +++++++++++++++++++++--------------------- test/traversals/dfs.jl | 12 +++++++++ 2 files changed, 43 insertions(+), 30 deletions(-) diff --git a/src/traversals/dfs.jl b/src/traversals/dfs.jl index 35dd1d54c..29a9aa41e 100644 --- a/src/traversals/dfs.jl +++ b/src/traversals/dfs.jl @@ -87,36 +87,37 @@ 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 - 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 - for n in outneighbors(g, u) - # we hit a loop when reaching back a vertex of the main path - if vcolor[n] == 2 - 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 - pop!(vertex_stack) - pushfirst!(verts, u) - end - end - end - return verts + # 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) + while !isempty(vertex_stack) + u = vertex_stack[end] + 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] == 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 + push!(vertex_stack, n) + end + end + else + pop!(vertex_stack) + # 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 + return verts end """ diff --git a/test/traversals/dfs.jl b/test/traversals/dfs.jl index 187b12fcb..f52a62c6c 100644 --- a/test/traversals/dfs.jl +++ b/test/traversals/dfs.jl @@ -23,6 +23,13 @@ 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) + @testset "dfs_tree" begin for g in testdigraphs(g5) z = @inferred(dfs_tree(GenericDiGraph(g), 1)) @@ -45,6 +52,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 From b5736befedfcc51f58f3d96d30514bbd451bd8f7 Mon Sep 17 00:00:00 2001 From: etienneINSA Date: Wed, 5 Jul 2023 15:53:13 +0200 Subject: [PATCH 5/8] fix is_cyclic --- src/traversals/dfs.jl | 12 ++++++------ test/traversals/dfs.jl | 6 ++++++ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/traversals/dfs.jl b/src/traversals/dfs.jl index 29a9aa41e..d518da675 100644 --- a/src/traversals/dfs.jl +++ b/src/traversals/dfs.jl @@ -38,24 +38,24 @@ end 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 diff --git a/test/traversals/dfs.jl b/test/traversals/dfs.jl index f52a62c6c..a77f649b1 100644 --- a/test/traversals/dfs.jl +++ b/test/traversals/dfs.jl @@ -29,6 +29,8 @@ 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) @@ -96,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 From 37113df363316ec5bc4c461cc442aab4bd632e2e Mon Sep 17 00:00:00 2001 From: Etienne dg Date: Wed, 5 Jul 2023 18:15:43 +0200 Subject: [PATCH 6/8] fix indentation --- src/traversals/dfs.jl | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/traversals/dfs.jl b/src/traversals/dfs.jl index d518da675..03446b040 100644 --- a/src/traversals/dfs.jl +++ b/src/traversals/dfs.jl @@ -87,11 +87,11 @@ 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 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) + # 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) while !isempty(vertex_stack) @@ -116,8 +116,8 @@ function topological_sort_by_dfs end end end end - end - return verts + end + return verts end """ From 037383739de68e224fdc9ba79508065f1f400886 Mon Sep 17 00:00:00 2001 From: Etienne dg Date: Wed, 5 Jul 2023 18:19:06 +0200 Subject: [PATCH 7/8] fix indentation --- src/traversals/dfs.jl | 48 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/traversals/dfs.jl b/src/traversals/dfs.jl index 03446b040..a3891995b 100644 --- a/src/traversals/dfs.jl +++ b/src/traversals/dfs.jl @@ -92,30 +92,30 @@ function topological_sort_by_dfs end verts = Vector{T}() vertex_stack = Vector{T}() for v in vertices(g) - vcolor[v] != 0 && continue - push!(vertex_stack, v) - while !isempty(vertex_stack) - u = vertex_stack[end] - 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] == 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 - push!(vertex_stack, n) - end - end - else - pop!(vertex_stack) - # 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 + vcolor[v] != 0 && continue + push!(vertex_stack, v) + while !isempty(vertex_stack) + u = vertex_stack[end] + 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] == 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 + push!(vertex_stack, n) + end + end + else + pop!(vertex_stack) + # 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 return verts end From c0d021d6f456c2adaf5b38bd522510f06687637c Mon Sep 17 00:00:00 2001 From: Etienne dg Date: Thu, 6 Jul 2023 09:57:10 +0200 Subject: [PATCH 8/8] Update comment --- src/traversals/dfs.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/traversals/dfs.jl b/src/traversals/dfs.jl index a3891995b..03eaca158 100644 --- a/src/traversals/dfs.jl +++ b/src/traversals/dfs.jl @@ -32,7 +32,7 @@ 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) @@ -87,7 +87,7 @@ 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 in the current dfs path, 2 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}()