fix is_cyclic

[aurorarossi]

issue #34, now is_cyclic works for undirected connected and disconnected graphs.

[codecov]

Codecov Report

Merging #168 (260632c) into master (a3fb98b) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   97.54%   97.47%   -0.07%     
==========================================
  Files         109      109              
  Lines        6314     6349      +35     
==========================================
+ Hits         6159     6189      +30     
- Misses        155      160       +5     

[gdalle]

Thanks for your contribution! This is a good first step, although I have questions about the performance. Can we do better on that point? The rest is mostly details

[gdalle]

Is there a way to do this without calling induced_subgraph? Judging by the implementation this function might lead to significant overhead, so I'm wondering if a more efficient option exists

[etiennedeg]

The most performant would be to use the same BFS than for the directed case, but without taking the edge from where we come from. This allows the algorithm to break early if it meets a cycle. By calling connected_components, we do a full BFS everytime...

[Liozou]

Hi and thank for your contribution! Here are some comments, mostly about code style, except for the @inferred note in the test which is a faulty test.
I'm not a maintainer of Graphs.jl so feel free to ignore the rest if you disagree, although I do believe it makes the code cleaner.

[Liozou]

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

[Liozou]

Suggested change

	if vertex_state[v] == unvisited
	if unvisited[v]

[Liozou]

Suggested change

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

[aurorarossi]

I noticed that sometimes some checks fail, in particular the failing is caused by test of test/traversals/diffusion.jl:120. It is a file that I didn't modify. Should I open an issue?

[Liozou]

Thanks a lot @aurorarossi for sticking with my many comments! A PR review process can be quite lengthy...
And indeed, after thinking a bit, I believe there is an alternative implementation that bypasses the creation of the extra parent array by keeping the only relevant parent directly in the S list, like so:

@traitfn function is_cyclic(g::AG::(!IsDirected)) where {T, AG<:AbstractGraph{T}}
    visited = falses(nv(g)) 
    for v in vertices(g)
        visited[v] && continue
        visited[v] = true
        S = [(v,v)] # the first v is a dummy "parent"
        while !isempty(S)
            parent, w = pop!(S)
            for u in neighbors(g, w)
                u == parent && continue
                visited[u] && return true
                visited[u] = true
                push!(S, (w, u))
            end
        end
    end
    return false
end

I believe it should be faster than your proposed PR, but we should benchmark it to check. Would you know how to generate cyclic and acyclic graphs to do so?
If it's faster, the code above can be adapted with your @enum if that's preferred. And if it's slower, then your current PR looks good to me as it is.

Regarding the irrelevant failing test, I guess it is a good idea to open an issue to keep track of that separately (but I'm not a maintainer of Graphs.jl so my word is just my opinion).

[aurorarossi]

@etiennedeg now the version of the algorithm is faster. I would like to thank @Liozou for the help!

[aurorarossi]

@matbesancon

[simonschoelly]

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).

[simonschoelly]

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)

[simonschoelly]

Can we also add to the function documentation, what the behavior is, for undirected graphs, especially with respect to self-loops?

[simonschoelly]

Awesome, thanks a lot!

[simonschoelly]

@gdalle Can I merge this? Github blocks me from merging, as long you have ongoing requests.