[ALGO]Bron-Kerbosch with pivoting #139
Conversation
Corrected trailling slashes
- relative railway example - corrected png files
- Added test cases and docs
Corrected doc file
- algorithm - test - documentation
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
There was a problem hiding this comment.
Hi @Hromz, as always great work 🚀
Hereby some feedback, please let me know what you think.
I really like how you extracted some of the set operations (intersection) into a function. It is super unfortunate that C++ or the stl does not give this for free. In a follow-up, it would be cool if we can extract such set operations to free functions and put them in a central place in the library which we can use as an extension to the stl. Out of scope for this PR but just some food for thought :)
| using graph_types = testing::Types<undirected_graph<int, int>>; | ||
| TYPED_TEST_SUITE(BronKerbosch, graph_types); |
There was a problem hiding this comment.
[minor]: If we only have one graph type we are testing, there is no real need for a TYPED_TEST_SUITE, instead we could just write the type directly in the tests. However, if you do want to test this for different graph types, we now have predefined type collections you can use here in <utils/fixtures/fixtures.h>.
| TYPED_TEST(BronKerbosch, RandomGraphCheckRunTime) { | ||
| using graph_t = typename TestFixture::graph_t; | ||
| graph_t graph{}; | ||
|
|
||
| std::vector<vertex_id_t> vertices; | ||
|
|
||
| for (int i = 1; i <= 70; ++i) { | ||
| vertices.push_back(graph.add_vertex(i)); | ||
| } | ||
|
|
||
| std::random_device dev; | ||
| std::mt19937 rng(dev()); | ||
| std::uniform_int_distribution<std::mt19937::result_type> rand_vertex(0, 69); | ||
|
|
||
| for (int i = 0; i < 1360; ++i) { | ||
| graph.add_edge(vertices[rand_vertex(rng)], vertices[rand_vertex(rng)], 1); | ||
| } | ||
|
|
||
| auto start = std::chrono::high_resolution_clock::now(); | ||
| auto cliques = bron_kerbosch(graph); | ||
| auto stop = std::chrono::high_resolution_clock::now(); | ||
|
|
||
| int expected_time = 900; | ||
|
|
||
| ASSERT_TRUE(expected_time >= | ||
| duration_cast<std::chrono::milliseconds>(stop - start).count()); | ||
| } |
There was a problem hiding this comment.
Great idea to measure the runtime performance 👍🏻
Not sure if you saw, but we are also using the google benchmark framework in /perf, where we get nice functionalities such as repeated executions of the algorithm after which the average runtime (and memory usage) is reported. There we can also nicely parametrize the benchmark to test for increasingly large graphs.
Such a benchmark might be a better suited place to measure runtime performance of our algorithms than in unit tests. As such I would propose to remove this unit test and to create a follow-up issue in which we can add the benchmark.
|
Hey @bobluppes! Thanks for your comments, with each PR I'm discovering something new. :) I corrected the code according to your comments. Renamed folder from "Clique" to "Clique-detection". Deleted the runtime test and added two more tests. |
Added algorithm, test and documentation.
In the end I got two implementations of the algorithm, but I decided to stick with the unordered_set implementation. Second one was bitwise approach with ULL implementation, unfortunately it didn't pass the runtime test (I guess I missed something in the implementation). In the future I would like to add bitwise approach and for sparse graph with vertex ordering appraoch.
Looking forward for your comments and suggestions!
closes #95