BFS in *.Algorithm modules

[jitwit]

First draft. Took a stab at adding bfs search. I went off the api for the dfs procedures and used sets from containers to keep track of state. The unfoldTreeM_BF uses a sequence under the hood for the queue.

If this looks ok, I take it the next steps would be to add tests and documentation? Otherwise, at least in the case of AdjacencyIntMaps, perhaps it would be worth using Arrays (as Data.Graph does)?

Joseph

[snowleopard]

@jitwit Many thanks for the PR! I'm currently travelling and have a few other PRs to review, but I'll try to come back to you soon!

In the meanwhile, have a look at the discussion in an unfinished PR #185, which was also about BFS: perhaps, you'll find something useful there.

[jitwit]

Thanks for the quick reply and no worries if it takes a while to get around to reviewing the changes!

I also incorporated tests for the bfs procedures. Again, I went off the respective dfs ones, but ensured the behavior is different where it should be. I also included the cases from the discussions in PR #185 , to indicate things are working as expected.

[jitwit]

I ran some benchmarks with criterion on Knuth's words graph from the stanford graph base, which has 34027 edges and 5757 vertices.

https://gist.github.com/jitwit/e19c2bc29126782831531755523d919b

I have two implementations of bfs right now, one using a Set/IntSet to keep track of visited vertices in the *.Algorithm modules, the other using a STUArray s Int Bool in the Data.Graph.Typed module.

The first group of benchmarks is done on the Graph String, the second on an isomorphic Graph Int copy of the first.

IntMap/Sets are quite fast, so the current dfs implementation which uses them actually does better than converting to the KL representation then running Data.Graph's dfs (~20ms vs ~30ms). The bfs implementation builds a bfs forest almost as fast as Data.Graph.dfs builds a dfs forest called directly on an Array Vertex [Vertex] (~7ms vs ~5ms).

For, AdjacencyMaps it seems like it's still better to convert to KL representation on DFS.

The implementation using STUArray s Int Bool was surprisingly slow. The overhead of converting to KL representation may be high or I may be doing something silly.

I build up all the graphs and run deepseq on them before running the benches, which I believe is the right way to do it, but let me know if I'm doing anything wrong!

[jitwit]

Compares well against fgl too!

https://gist.github.com/jitwit/0c2d136cadc7a32f090fb98dda338bab

[jitwit]

Hi @snowleopard. Does the bfs implementation look reasonable? It uses Data.Tree's monadic unfold to build the forest.

I've added tests and documentation with complexity analysis. This implementation seems to perform better than fgl's, according to some criterion benchmarks I ran (see gists above). Thanks!

[snowleopard]

@jitwit Many thanks for finishing off the PR and for benchmarking! Apologies once again for being so slow.

I like your implementation, but I left a few minor comments -- please have a look.

If I understand your benchmarks correctly, your BFS implementation is much faster than the current DFS one. In this case, shall we switch reachable to using BFS? Or do you think we could speed up DFS as well?

[jitwit]

In a few words: Using bfs for reachable is probably a good idea. Also, I think it may be possible to improve depth first search, but the current attempt gets mixed results!

In many words: Yeah that seems to be the case. I think it's more accurate to say that new bfs is much faster for AdjacencyIntMaps, and somewhat faster for AdjacencyMap a.

I ran yet more benches, and also tried reachable with bfs as well. For the example graph (n = 5757, m = 34027), bfs-reachable seems to be ~3x faster than the current reachable on AdjacencyIntMap, and around 50% faster for AdjacencyMap Text. So it could be the way to go!

I think that breadth first search is performing better for two reasons. One is that once a node is enqueued, it can be excluded from further consideration, whereas with depth first, queued nodes are not necessarily in the dfs forest. The second reason is avoiding overhead of converting representations.

The first reason makes the attempt at re-implementing depth first search on AdjacencyMap a bit awkward, using Data.Tree's unfoldTreeM. I haven't come up a better solution than returning a Forest (Maybe a), then pruning this to aForest a. Basically, nodes are enqueued when "white" or "gray". On exit, if still "gray" a Just a is put in the forest, or if "black", a Nothing node is. I might be missing something obvious? The attempt is in the gist after imports.

According to previous benchmarks, that dfs attempt was faster for a good portion of graphs, but not all. eg K_1000 got horrible results.

https://gist.github.com/jitwit/80447786c00a43d757e8d8de26549b8f

[snowleopard]

@jitwit Sounds good, let's switch to using BFS for reachable then! We should be able to see some speed up on the CI performance suite immediately (https://travis-ci.org/snowleopard/alga/jobs/558141381).

I've been also thinking about making bfs a bit more useful for a larger proportion of users, and I think that perhaps we could omit the concat step, switching to bfs :: Ord a => [a] -> AdjacencyMap a -> [[a]], thus returning BFS levels. It's easy to apply concat to the latter if need be, but going in the opposite direction is trickier (e.g. one needs to know the function levels from Data.Tree). What do you think?

[snowleopard]

P.S.: We could have a look at optimising DFS separately.

[jitwit]

Hm! It seems reachable is hand rolled bench suite, as well as conversion to AdjacencyMap. I was perplexed to see 1.0 new vs old in the Bench job.

bench suite: https://github.com/haskell-perf/graphs/blob/master/bench/Alga/Graph.hs
build job: https://api.travis-ci.org/v3/job/558343849/log.txt

[jitwit]

@snowleopard I also definitely agree with returning [[a]] from bfs. I initially went [a] to mirror the API from Data.Graph.

I'll try to think more about optimizing dfs. I have a hunch that even if dfs proves hard to match performance-wise, topological sort directly on AdjacencyMaps may possibly work?

[jitwit]

Maybe I should add a comment in the documentation about concat . bfs x returning the vertices in bfs-order?

[snowleopard]

Maybe I should add a comment in the documentation about concat . bfs x returning the vertices in bfs-order?

@jitwit Yes, that would be useful! Perhaps, you can even add it as an example, using the same bidirectional circuit graph.

[jitwit]

@snowleopard Hopefully I've not left any documentation/tests out of sync this time!

Also, I hope the documentation changes are reasonable? Wikipedia seems to define level structure mentioning undirected graphs, but no mention of directed ones, so I can edit if it goes against the standard definition.

https://en.wikipedia.org/wiki/Level_structure#cite_note-dps-1

[snowleopard]

@jitwit The performance suite has been fixed (thanks @nobrakal!) and now we have the following improvement (https://travis-ci.org/snowleopard/alga/jobs/558388854):

reachable: 0.78 (good)

That's pretty good! Note that this is benchmarking Algebra.Graph, so the increase in performance is masked somewhat by the time it takes to convert a Graph to AdjacencyMap.

[jitwit]

Cool! Nice to see the benefits play out

[snowleopard]

@jitwit I think it's pretty much ready for merge, but see a couple of more comments.

P.S.: Please add your name to AUTHORS.md if you like to be listed.

[jitwit]

@snowleopard Nice! Also, fingers crossed I didn't get any tests/docs out of sync. I tried to look carefully!

[snowleopard]

@jitwit Merged, thank you! If you are not fed up with the long review times, it would be great if you could look into improving DFS algorithms, as discussed above :)

[jitwit]

@snowleopard nice! haha, definitely, I'll start trying some ideas for DFS