[BENCH] Bron-Kerbosch clique detection #157
Conversation
Corrected trailling slashes
- relative railway example - corrected png files
- Added test cases and docs
- algorithm - test - documentation
Codecov ReportAll modified lines are covered by tests ✅ see 4 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
| std::vector<graaf::edge_id_t> clique{}; | ||
| clique.reserve((clique_size * (clique_size - 1)) / 2); | ||
|
|
||
| // Constructing a clique | ||
| for (size_t i{start_vertex}; i < end_vertex; ++i) { | ||
| for (size_t j{i + 1}; j < end_vertex; ++j) { | ||
| clique.emplace_back(vertices[i], vertices[j]); | ||
| } | ||
| } | ||
|
|
||
| for (auto& edge_t : clique) { | ||
| graph.add_edge(edge_t.first, edge_t.second, 1); | ||
| } |
There was a problem hiding this comment.
What is the need of storing the edges in the vector clique first? Can we not directly add them to the graph? i.e.
| std::vector<graaf::edge_id_t> clique{}; | |
| clique.reserve((clique_size * (clique_size - 1)) / 2); | |
| // Constructing a clique | |
| for (size_t i{start_vertex}; i < end_vertex; ++i) { | |
| for (size_t j{i + 1}; j < end_vertex; ++j) { | |
| clique.emplace_back(vertices[i], vertices[j]); | |
| } | |
| } | |
| for (auto& edge_t : clique) { | |
| graph.add_edge(edge_t.first, edge_t.second, 1); | |
| } | |
| // Constructing a clique | |
| for (size_t i{start_vertex}; i < end_vertex; ++i) { | |
| for (size_t j{i + 1}; j < end_vertex; ++j) { | |
| clique.emplace_back(vertices[i], vertices[j]); | |
| graph.add_edge(vertices[i], vertices[j], 1); | |
| } | |
| } |
There was a problem hiding this comment.
Oh my bad, I overconfused myself.
|
|
||
| static void bron_kerbosh_two_vertices_cliques(benchmark::State& state) { |
There was a problem hiding this comment.
If I understand the code correctly, we create a graph with a linear path of connected vertices:
a -- b -- c -- ...
This was not immediately clear to me when looking at the benchmark name. What do you think about calling this something like bron_kerbosch_linear_graph or bron_kerbosch_two_vertices_per_clique? To make it extra clear we could also add a small comment on top of each benchmark explaining the setup.
There was a problem hiding this comment.
Agreed, that definitely will be a better explanation of what code does.
| for (auto _ : state) { | ||
| for (size_t i{0}; i < number_of_edges; ++i) { | ||
| graph.add_edge(vertices[i], vertices[i + 1], 1); | ||
| } | ||
| } | ||
|
|
||
| graaf::algorithm::bron_kerbosch(graph); | ||
| state.SetComplexityN(state.range(0)); |
There was a problem hiding this comment.
Looking at the docs the to-be-benchmarked code should go into the body of this for-loop. I therefore think we should add the edges outside of the loop and call bron_kerbosch inside.
In order to make sure the compiler does not optimize the call to the algorithm away (since the result is unused) it would also be good to add benchmark::DoNotOptimize:
| for (auto _ : state) { | |
| for (size_t i{0}; i < number_of_edges; ++i) { | |
| graph.add_edge(vertices[i], vertices[i + 1], 1); | |
| } | |
| } | |
| graaf::algorithm::bron_kerbosch(graph); | |
| state.SetComplexityN(state.range(0)); | |
| for (size_t i{0}; i < number_of_edges; ++i) { | |
| graph.add_edge(vertices[i], vertices[i + 1], 1); | |
| } | |
| for (auto _ : state) { | |
| auto result = graaf::algorithm::bron_kerbosch(graph); | |
| benchmark::DoNotOptimize(result); | |
| } | |
| state.SetComplexityN(state.range(0)); |
There was a problem hiding this comment.
Same for the other benchmarks, maybe you could also take a look there
| for (size_t i{0}; i < number_of_edges; ++i) { | ||
| for (size_t j{i + 1}; j < number_of_edges; ++j) { | ||
| if (i != j && !graph.has_edge(vertices[i], vertices[j])) | ||
| graph.add_edge(vertices[i], vertices[j], 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Looking at these for loops, would the condition in the if-statement ever evaluate to false? I think this is equivalent to:
for (size_t i{0}; i < number_of_edges; ++i) {
for (size_t j{i + 1}; j < number_of_edges; ++j) {
graph.add_edge(vertices[i], vertices[j], 1);
}
}Which in turn looks pretty similar to the add_clique function. Could we therefore simply do:
add_clique(graph, vertices, vertices.front(), number_of_edges);There was a problem hiding this comment.
^^ Oh, how I mised that! Thanks for emphasizing this!
|
|
||
| size_t clique_size = 4; |
There was a problem hiding this comment.
Should the clique_size not be equal to 3 if we want to make triangle cliques?
There was a problem hiding this comment.
My bad with naming, basically, it's a triangle with a dot inside (4 vertex clique).
|
|
||
| size_t clique_size = 25; |
There was a problem hiding this comment.
We could also do this in a follow-up, but looking at these parameters it looks like we can pull out the clique_size as a second parameter. Then we might be able to consolidate a few of these benchmarks. I.e.
BENCHMARK(bron_kerbosh_connected_cliques)
->Ranges({{100, 10000}, {3, 100}}) // total number of edges, clique size
->Unit(benchmark::kMillisecond)
->Complexity();There was a problem hiding this comment.
If you want I would like to add this in current PR!
|
|
||
| // Generating random number of vertices (1- 30) for a clique | ||
| std::random_device dev; | ||
| std::mt19937 rng(dev()); | ||
| std::uniform_int_distribution<std::mt19937::result_type> random_clique_size(1, | ||
| 30); |
There was a problem hiding this comment.
I am always a little hesitant adding such non deterministic benchmarks, as it makes comparing results difficult. It might still be nice to have since we use it to determine the runtime complexity as well, just something to be aware of.
Added a benchmark for the Bron-Kerbosch algorithm. This implementation consists of sparse, dense, custom, and random graphs. Additionally, I added ms. instead of nn and BigO notation. If you think that is inappropriate here, let me know and I will remove it.