Support for edge multiplicities in brandes_betweenness_centrality

[javidcf]

Ticket #9935
Implemented support for edges multiplicities in betweenness calculation. As explained in the ticket, the algorithm variation is taken from Ulrik Brandes' paper On variants of shortest-path betweenness centrality and their generic computation, section 3.8, algorithm 11. Please note that this paper has a mistake on this algorithm, as explained in Brandes' publications web page - this patch implements the corrected algorithm.
Note this implementation uses static_property_map and make_static_property_map, which are currently in the develop branch of the property_map library.

Note: This PR cause compilation error when using non_distributed_betweenness centrality from graph-parallel. PR #2 in that library fix these errors, while adding the multiplicity feature to that version of the algorithm.

[javidcf]

If this pull request gets accepted, I can also try to implement this feature for the graph_parallel library.

[Belcourt]

I pulled this code into develop and attempted to build it, graph_parallel fails to compile on Darwin with gcc, here's the error.

In file included from ../libs/graph_parallel/test/distributed_csr_algorithm_test.cpp:30:
../boost/graph/distributed/betweenness_centrality.hpp:1453:18: error: use of class template 'detail::graph::brandes_unweighted_shortest_paths' requires template arguments
detail::graph::brandes_unweighted_shortest_paths shortest_paths;
^
../boost/graph/betweenness_centrality.hpp:171:10: note: template is declared here
struct brandes_unweighted_shortest_paths
^
In file included from ../libs/graph_parallel/test/distributed_csr_algorithm_test.cpp:30:
../boost/graph/distributed/betweenness_centrality.hpp:1481:18: error: too few template arguments for class template 'brandes_dijkstra_shortest_paths'
detail::graph::brandes_dijkstra_shortest_paths shortest_paths(weight_map);
^
../boost/graph/betweenness_centrality.hpp:134:10: note: template is declared here
struct brandes_dijkstra_shortest_paths
^
1 warning and 2 errors generated.

"g++"  -ftemplate-depth-128 -O0 -fno-inline -Wall -g -dynamic -gdwarf-2 -fexceptions -fPIC  -DBOOST_ALL_NO_LIB=1 -DBOOST_SYSTEM_DYN_LINK=1  -I".." -I"/Users/kbelco/Projects/openmpi-1.4.5/gcc-4.2.1/include" -c -o "../bin.v2/libs/graph_parallel/test/distributed_csr_algorithm_test-1.test/darwin-4.2.1/debug/distributed_csr_algorithm_test.o" "../libs/graph_parallel/test/distributed_csr_algorithm_test.cpp"

Could you take a look at this and, ideally, submit a PR against graph_parallel as well? I can help with the parallel (MPI) part.

sanj2010-253-180:status kbelco$ gcc -v
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 5.1 (clang-503.0.40) (based on LLVM 3.4svn)
Target: x86_64-apple-darwin12.5.0
Thread model: posix

[Belcourt]

I noticed that there's no test case for this feature, could you put together a test case and a little documentation for this as well?

[javidcf]

@Belcourt Thanks for your comments. I haven't really used graph_parallel, and I wasn't aware that it used internal structs of graph. I'll look at it and try to make a fix so everything works together.

Also, you're right, I should post a test case for this. To be honest, I wasn't completely sure there would be any interest in merging it. Anyway, the HTML documentation was updated to include the multiplicity map parameter.

[javidcf]

@Belcourt I've added a quick fix for the compilation issue in the develop branch of my fork of graph-parallel. I have not posted a pull request as I would like to actually introduce the multiplicity feature, but I'll do if I can't work it out.

[javidcf]

@Belcourt I've put a PR in graph-parallel to fix the errors you reported and to add the edge multiplicities to non_distributed_betweenness_centrality.

[Belcourt]

In doc/betweenness_centrality.html, the MultiplicityMap parameter is missing from the template parameter list. Further down, would you fix the several places where:

negative_edge exception is one of the edges

should be if, not is. Also, since MultiplicityMap checks for non-postive, perhaps a different exception rather than using negative_edge, e.g. nonpostive_edge or something (also change in header)?

For the brandes_dijkstra_visitor, I see you've changed the order of the arguments, could you add your MultiplicityMap to the end of the template, function, and members list? Why change the argument order?

Same question for brandes_betweenness_centrality_dispatch2, why put the MultiplicityMap ahead of the VertexIndexMap?

Sorry I'm so late getting back to this.

[javidcf]

@Belcourt Thank you for the feedback. I'll fix the doc right now.

I wasn't sure about creating a new exception for nonpositive, but if you think it's okay I'll do it. I was thinking about making it a superclass of negative_edge (as it is a supercase of it), let me know if you do not agree.

About the parameters order, I was trying to keep the weight and the multiplicity "near", because they seemed kind of "similar" parameters, both carrying actual edge information, while incoming, distance, path_count and vertex_index seemed more like auxiliary or output information. However, I have no problem in putting it in the end.

[anadon]

I'm helping out with the PR backlog. Looks like you have a feature additions. Given the age of this PR, I'd like you to chime in @javidcf and see where things stand. This is to let you know and help me prioritize PR's.

[javidcf]

@anadon Hey, sorry I missed this. This is an old one indeed, I implemented it because I needed the feature for a particular thing I did four years ago and it was missing, it worked but tbh I don't remember much of it. It seems I did make the changes Belcourt asked (order of parameters in a function and the documentation), and graph_parallel should also work with the corresponding changes in the referenced PR. What I think is missing is the testing, I think I had a look at it and didn't find it obvious how to do it, so I thought I would do it later at some point but never got around to it. Shameful of me :/

[anadon]

@javidcf Should this PR be closed, then?

[javidcf]

@anadon Honestly I don't really have time much now to relearn all my way through boost.graph, verify everything is fine, add the test case or whatever bits are missing and merge it with the current version. The feature is legit but I suppose maybe not super-demanded, so if there is no particular interest on it you can close it (tbh I didn't even think it was still open). Also if it is actually needed at some point is not even that hard to implement, I mean most of the changes are scaffolding, the actual logic is rather simple I think. Btw if this PR is closed the corresponding one in graph_parallel should be closed as well then I suppose.

[anadon]

@jzmaddock This should probably be closed unless someone has a similar need in future. It is a judgement call though, and Boost proper may want to keep this kind of PR around. I'm not sure.

[jzmaddock]

I think I'm not qualified to judge - for now I would be inclined to leave open and see if anyone else adds a "me too, please implement this!". However, given that we have merge conflicts, no tests, and are blocked waiting for the graph_parallel PR to be merged this one requires a fair bit of work that's probbaly beyond the CMT IMO.