Implement bridge #1105

piever · 2019-01-09T21:31:10Z

This implements a function to compute all bridges of a graph. My main doubt is what should be the output format of this (returning a subset of edges). I have opted for an array of CartesianIndex with the indices of the edges (which is what one generally gets when doing findall on a matrix), but maybe simply a list of Tuple or a tuple of arrays would make more sense.

I also plan to implement "edge-biconnected components" but I think that can be done in a separate PR.

sbromberger · 2019-01-09T22:00:47Z

What do you mean by "indices of the edges"? We don't have edge indices in LG.

jpfairbanks · 2019-01-09T22:08:01Z

Based on the code, it looks like edge indices means the pair of vertices (u,v).

push!(state.bridges, (v, w)) I like idea of thinking of an edge as a Cartesian index, but we don't do it anywhere else so I think returning a Vector{Edge} is the right answer for now.

codecov · 2019-01-10T00:26:10Z

Codecov Report

Merging #1105 into master will increase coverage by <.01%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #1105      +/-   ##
==========================================
+ Coverage   93.86%   93.87%   +<.01%     
==========================================
  Files          90       91       +1     
  Lines        4354     4375      +21     
==========================================
+ Hits         4087     4107      +20     
- Misses        267      268       +1

piever · 2019-01-10T00:31:18Z

Ok, I've replaced CartesianIndex(u, v) with Edge(u, v). I thought it was nice to be able to use the result directly as indices in a square matrix, but using Edge is definitely more consistent with the rest of the package.

One more question: if I use the Edge, should I just add Edge(i, j) with i<j? I'm currently adding both directions but I think that's against the convention in the package. EDIT: have updated the PR to only add one edge per bridge.

piever · 2019-01-10T10:40:30Z

It should be good to go/review on my side: the reduction in test coverage seems to be an artefact as the untested line is between two lines that are tested.

simonschoelly · 2019-01-10T12:49:48Z

src/biconnectivity/bridge.jl

+"""
+    bridge(g)
+Compute the [bridges](https://en.m.wikipedia.org/wiki/Bridge_(graph_theory))
+of a connected graph `g` and return an array containing all bridges.


As this can be done in a single sentence, I think we should describe here what a bridge is.

simonschoelly · 2019-01-10T12:52:29Z

src/biconnectivity/bridge.jl

+ Edge 1 => 2
+```
+"""
+function bridge(g::AbstractGraph)


I would call this bridges instead.
g should be restricted to undirected graphs, unless we can define what the bridge of a directed graph is.
Also, we should restrict g to integer eltypes here, as the function will not work otherwise.

bridge is a bit ambiguous for directed graphs, better to restrict the dispatch.

Concerning the name, I agree bridges makes more sense. I wanted to be consistent with articulation (which should maybe be articulations instead?), but maybe it's better to call this bridges and deprecate articulation to articulations in the future.

I think articulation is shorthand for articulation_points. But I agree that this should be bridges.

I see, yes this should definitely be bridges.

I'm a bit unsure what's the correct dispatch, because it is not clear to me that all graphs types defined in various packages are subtypes of SimpleGraphs.AbstractSimpleGraph. Maybe easier to accept all AbstractGraph{<:Integer} and throw an argument error if the graph is directed.

We use the package SimpleTraits.jl for dispatching on directed/undirected graphs. For an example on how to do that, you can have a look at the file LightGraphs/src/community/cliques.jl. The function maximal_cliques is restricted to undirected graphs there.

I wouldn't bother with vertices that are not integers if there is not a simple and performant way to do such a thing. As far as I know, there is no package that uses such a graph yet and if there was, that person would have to implement many other functions from LightGraphs anyway, as they will not work out of the box.

simonschoelly · 2019-01-10T12:53:30Z

src/biconnectivity/bridge.jl

+
+function Bridges(g::AbstractGraph)
+    n = nv(g)
+    T = typeof(n)


You can use eltype(g) here.

simonschoelly · 2019-01-10T12:55:35Z

src/biconnectivity/bridge.jl

+    state.depth[v] = state.id
+    state.low[v] = state.depth[v]
+
+    for w in outneighbors(g, v)


Maybe put an @inbounds in front of to the loop.

simonschoelly · 2019-01-10T13:12:27Z

This recursive algorithm is great, but in the future we might have to replace it with a iterative one, as this code already gives me a stack overflow: bridge(PathGraph(5 * 10^4)).

Don't worry about coverage, that is broken at the moment.

piever · 2019-01-11T17:47:04Z

This recursive algorithm is great, but in the future we might have to replace it with a iterative one, as this code already gives me a stack overflow: bridge(PathGraph(5 * 10^4)).

I see, if the depth first search graph is very deep things just fail... Ideally we could have a DFS iterator that one would use to code this algorithms? Maybe even a "Tarjan" iterator.

sbromberger · 2019-01-11T18:08:23Z

Ideally we could have a DFS iterator that one would use to code this algorithms?

Well, we've played with that in the past but the problem is that calling generic visiting / opening / closing functions makes things horribly slow so we specialze the DFS for each algorithm that needs it. See
https://github.com/JuliaGraphs/LightGraphs.jl/blob/master/src/traversals/dfs.jl for some examples.

Edited to add: we've made a conscious decision in the past to remove functions that don't scale to large graphs. I don't see us adding any functions that can't handle, say, 1 million nodes / 100 million edges without crashing.

jpfairbanks · 2019-01-11T18:16:37Z

I think that code was slow because of pre v0.6 implementation stuff. I think you could write a generic visitor code now and it wouldn't be slower. The old way was to pass the hooks in as arguments. But I think the new Julia 1.0 way is to define a VisitorType struct and then overload these functions for the api.

prehook(t::VisitorType, g, v) 
onhook(t::VisitorType, g, v) 
posthook(t::VisitorType, g, v)

sbromberger · 2019-01-11T18:18:46Z

@jpfairbanks I'd be really interested in bringing back the traversal code if we can show that it's as fast. I'll open up an issue and maybe one of the GSOC candidates can use it to get familiar w/ our codebase.

piever · 2019-01-11T19:45:47Z

Should have implemented most of the feedback. I'm really curious about the VisitorType struct for iteration (from a link in Slack there was a TarjanVisitor that got deleted - I suppose for performance reasons in older Julia - but it would have been handy), but I feel that belongs to a separate PR and when we have an efficient implementation of that I can see if I can simplify the code here.

simonschoelly · 2019-01-18T14:19:39Z

I think this could be merged. @abhinavmehndiratta is working on an iterative version of the similar articulation algorithm and could maybe later transfer his code to this.

add newlines between signature and start of doc

* implement bridge computation * added tests * fix reference * fix docstring * change to Edge * simplify test * fix tests * fix tests for real * just add one edge * remove unnecessary lines * implement suggestions * rename to bridges * added check for undirectedness * use dispatch on trait IsDirected * update docs * Update bridge.jl add newlines between signature and start of doc

Pietro Vertechi added 4 commits January 9, 2019 20:42

implement bridge computation

07c7740

added tests

f033e35

fix reference

aec8d87

fix docstring

f4033c3

Pietro Vertechi added 2 commits January 10, 2019 00:23

change to Edge

e2ee876

simplify test

4c0904d

fix tests

8d4fb6d

Pietro Vertechi added 3 commits January 10, 2019 00:36

fix tests for real

9c24830

just add one edge

3c64113

remove unnecessary lines

c863e88

simonschoelly reviewed Jan 10, 2019

View reviewed changes

Pietro Vertechi added 5 commits January 11, 2019 19:18

implement suggestions

134c2b6

rename to bridges

3efd066

added check for undirectedness

6a68f5e

use dispatch on trait IsDirected

2690178

update docs

75842d3

simonschoelly approved these changes Jan 18, 2019

View reviewed changes

Merge branch 'master' into pv/bridge

b396b98

Update bridge.jl

c737a86

add newlines between signature and start of doc

sbromberger approved these changes Jan 18, 2019

View reviewed changes

sbromberger merged commit ee0db34 into sbromberger:master Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement bridge #1105

Implement bridge #1105

piever commented Jan 9, 2019 •

edited

Loading

sbromberger commented Jan 9, 2019

jpfairbanks commented Jan 9, 2019

codecov bot commented Jan 10, 2019 •

edited

Loading

piever commented Jan 10, 2019 •

edited

Loading

piever commented Jan 10, 2019

simonschoelly Jan 10, 2019

simonschoelly Jan 10, 2019

piever Jan 10, 2019

sbromberger Jan 11, 2019

piever Jan 11, 2019

simonschoelly Jan 11, 2019

simonschoelly Jan 10, 2019

simonschoelly Jan 10, 2019

simonschoelly commented Jan 10, 2019

piever commented Jan 11, 2019

sbromberger commented Jan 11, 2019 •

edited

Loading

jpfairbanks commented Jan 11, 2019

sbromberger commented Jan 11, 2019

piever commented Jan 11, 2019

simonschoelly commented Jan 18, 2019

Implement bridge #1105

Implement bridge #1105

Conversation

piever commented Jan 9, 2019 • edited Loading

sbromberger commented Jan 9, 2019

jpfairbanks commented Jan 9, 2019

codecov bot commented Jan 10, 2019 • edited Loading

Codecov Report

piever commented Jan 10, 2019 • edited Loading

piever commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonschoelly commented Jan 10, 2019

piever commented Jan 11, 2019

sbromberger commented Jan 11, 2019 • edited Loading

jpfairbanks commented Jan 11, 2019

sbromberger commented Jan 11, 2019

piever commented Jan 11, 2019

simonschoelly commented Jan 18, 2019

piever commented Jan 9, 2019 •

edited

Loading

codecov bot commented Jan 10, 2019 •

edited

Loading

piever commented Jan 10, 2019 •

edited

Loading

sbromberger commented Jan 11, 2019 •

edited

Loading