Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix is_cyclic #168

Merged
merged 18 commits into from
Sep 20, 2022
Merged

fix is_cyclic #168

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
c3b2561
fix is_cyclic
aurorarossi Jul 18, 2022
bc02277
Merge branch 'JuliaGraphs:master' into master
aurorarossi Aug 25, 2022
2024eb7
fix unconnected graphs
aurorarossi Aug 25, 2022
05cb957
fixed graph for test
aurorarossi Aug 26, 2022
f056203
better performance with dfs
aurorarossi Sep 1, 2022
2f02612
new version
aurorarossi Sep 7, 2022
2a21c58
test inferred corrected
aurorarossi Sep 8, 2022
c8ad5d7
removed empty line
aurorarossi Sep 8, 2022
f667c1f
better style in push
aurorarossi Sep 8, 2022
dda80ad
vector improved
aurorarossi Sep 8, 2022
7af46af
update parent
aurorarossi Sep 8, 2022
64486d8
Update src/traversals/dfs.jl
aurorarossi Sep 9, 2022
f2edf24
spaces
aurorarossi Sep 9, 2022
bc73585
Update src/traversals/dfs.jl
aurorarossi Sep 9, 2022
a1b9f9d
new version thanks to @Liozou
aurorarossi Sep 9, 2022
2a0de95
fixed tests and documentation self-loops
aurorarossi Sep 19, 2022
487dd1e
self loops undirected
aurorarossi Sep 19, 2022
260632c
Update src/traversals/dfs.jl
aurorarossi Sep 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions src/traversals/dfs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,35 @@
is_cyclic(g)

Return `true` if graph `g` contains a cycle.
aurorarossi marked this conversation as resolved.
Show resolved Hide resolved

### Implementation Notes
Uses DFS.
The algorithm uses a DFS.
"""
function is_cyclic end
@traitfn is_cyclic(g::::(!IsDirected)) = ne(g) > 0
@enum Vertex_state unvisited visited
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@enum Vertex_state unvisited visited

Since this enum is binary, I believe it's clearer to replace it by a vector of booleans. It's actually also slightly more memory-friendly since those can be nicely packed into a BitVector

@traitfn function is_cyclic(g::AG::(!IsDirected)) where {T, AG<:AbstractGraph{T}}
vertex_state = fill(unvisited, nv(g))
parent = zeros(T, nv(g))
for v in vertices(g)
if vertex_state[v] == unvisited
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if vertex_state[v] == unvisited
if unvisited[v]

S = T[v]
while !isempty(S)
w = pop!(S)
if vertex_state[w] == unvisited
vertex_state[w] = visited
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if vertex_state[w] == unvisited
vertex_state[w] = visited
if unvisited[w]
unvisited[w] = false

for u in neighbors(g, w)
if parent[w] != u
push!(S, u)
parent[u] = w
end
end
else
return true
end
end
end
end
return false
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me, but there is a small edge that we also have to consider: If an undirected graph has a self-loop (i.e. a vertex that is connected to itself), then this should also count as a cycle in my opinion. (or if not, at least we have to document this).

# 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))
Expand Down
7 changes: 5 additions & 2 deletions test/traversals/dfs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
g5 = SimpleDiGraph(4)
add_edge!(g5, 1, 2); add_edge!(g5, 2, 3); add_edge!(g5, 1, 3); add_edge!(g5, 3, 4)
gx = cycle_digraph(3)

gcyclic = SimpleGraph([0 1 1 0 0; 1 0 1 0 0; 1 1 0 0 0; 0 0 0 0 1; 0 0 0 1 0])
gtree = SimpleGraph([0 1 1 1 0 0 0; 1 0 0 0 0 0 0; 1 0 0 0 0 0 0; 1 0 0 0 1 1 1; 0 0 0 1 0 0 0; 0 0 0 1 0 0 0; 0 0 0 1 0 0 0])
@testset "dfs_tree" begin
for g in testdigraphs(g5)
z = @inferred(dfs_tree(g, 1))
Expand All @@ -26,9 +27,11 @@

@testset "is_cyclic" begin
for g in testgraphs(path_graph(2))
@test @inferred(is_cyclic(g))
@test !@inferred(is_cyclic(g))
@test @inferred(!is_cyclic(zero(g)))
end
@test @inferred(is_cyclic((gcyclic)))
@test @inferred(!is_cyclic(gtree))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a few more test-cases we have to cover. First, its often good to use graphs with different eltypes (here you use only Int) as a lot of errors happen when that is not the case.

Apart from that, it is often good to have these edge cases in your test:

  • A graph with zero vertices
  • A graph with isolated vertices
  • A graph with multiple components (this is already handled by your test case)
  • A graph with self-loops (we definitely need this test case here)

end

end