Added Karps Minimum Cycle Algorithm #281
Conversation
|
Thanks for taking the time to implement this. It looks good, but as I said earlier it will take a few rounds of feedback to get it to be consistent with existing BGL code. Please take a look at an existing similar algorithm such as Howard's cycle ratio algorithm and imitate what is there. There is a lot more to an algorithm than just the implementation, namely:
PS. One thing that I do not expect anyone to imitate from the existing code are named parameters. |
|
I marked this as a draft for now in the hope that it won't trigger the CI every time you push -- I'm still learning how this CI system works. When you have completed everything then mark it ready for review again. |
| namespace boost | ||
| { | ||
| template<typename Graph> | ||
| double karp_minimum_mean_cycle(Graph g) |
There was a problem hiding this comment.
Although it doesn't roll off the tongue very well, the paper does call it the "minimum cycle mean".
|
Thanks for looking into this. I will start the documentation, was just waiting for someone to approve of the code. |
|
Hi @GYuvanShankar , thanks, it's looking good! I'll start a review, in which I'll make some (or many) small comments in the code itself. |
|
When implementing algorithms, of any kind, it's important to be really familiar with the literature on them. In this case there is at least one paper that demonstrates a defect with Karp's algorithm and an alternative solution: https://www.cs.colostate.edu/~rmm/minCycleMean.pdf I recommend that you read widely on MCM and then come back with a discussion on which algorithm(s) you propose to implement and why. This is an interesting overview, although I have only glanced at it: https://www.researchgate.net/profile/Rajesh-Gupta-10/publication/2689582_An_Experimental_Study_of_Minimum_Mean_Cycle_Algorithms/links/546e071f0cf29806ec2e6dba/An-Experimental-Study-of-Minimum-Mean-Cycle-Algorithms.pdf |
| #ifndef BOOST_GRAPH_KARP_MINIMUM_CYCLE_MEAN_HPP | ||
| #define BOOST_GRAPH_KARP_MINIMUM_CYCLE_MEAN_HPP | ||
|
|
||
| #include <iostream> |
There was a problem hiding this comment.
We don't allow <iostream> in header files: it brings in a lot of names and static objects, etc.
| namespace boost | ||
| { | ||
| template<typename Graph> | ||
| double karp_minimum_cycle_mean(Graph g) |
There was a problem hiding this comment.
I think you'll want to take g by const reference.
| typename property_map<Graph, edge_weight_t>::type weight = get(edge_weight,g); | ||
| typedef typename boost::graph_traits<Graph>::out_edge_iterator out_edge_iterator; | ||
| BOOST_CONCEPT_ASSERT((GraphConcept< Graph >)); | ||
|
|
There was a problem hiding this comment.
A comment about this file in general, we don't have automatic Clang format running (yet), so please follow the convention in the rest of the library:
- whitespace around operators:
a + b,a = b, etc.
| </P> | ||
| <P> | ||
| This algorithm was described by Richard M. Karp in his paper | ||
| <A HREF="./dasdan-dac99.pdf">A characterization of the minimum cycle mean in a digraph</A></P> |
There was a problem hiding this comment.
Please give a proper citation of the paper.
|
It was a courageous and ambitious experiment at the time (20 something years ago) but it adds an awful lot of complexity and maintenance burden to the code for questionable benefit. In the long run, I would like to strip them out. I think we could achieve similar benefits, if wanted, from much simpler overloads and stronger types. |
Based on issue #272.
Implemented Karps Minimum Cycle Algorithm.