Add rank spanning branchings routine for possible inclusion in library. #82

mbradle · 2016-11-06T13:24:25Z

Hello,

I am opening a new version of this pull request (previous version). I've squashed all the development commits into a single commit (but have included the development history in the commit message).

The documentation file (rank_spanning_branchings.html) describes the added code (rank_spanning_branchings.hpp). I've included two examples (rank-branchings1.cpp and rank-branchings2.cpp), the second of which reads an input file (branching_input.txt). I've also included a test routine (rank_branchings_test.cpp). I added the examples and the test to the corresponding Jam files. These have small conflicts with the development branch that I thought it might be better for the maintainers to resolve.

Thanks for your consideration, and best wishes.

Brad Meyer

anadon · 2018-08-31T18:14:26Z

I'm helping out with the PR backlog. Looks like you have feature additions, new documentation, no test, and new examples will be needed. Please add tests for your added features. This is to let you know and help me prioritize PR's.

mbradle · 2018-09-02T20:21:05Z

Thanks for the update. There are two examples included in the pull request:

example/rank-branchings1.cpp

and

example/rank-branchings2.cpp

Once compiled, they are executed as:

./rank-branchings1

and (for example)

./rank-branchings2 branching_input.txt -10

I've included the compilation of these in the example/Jamfile.v2.

There is also a test:

test/rank_branchings_test.cpp

Once compiled, this can be executed as (for example)

./rank_branchings_test 4 10

I've included it in the test/Jamfile.v2 file.

I notice above that two Travis builds failed on my complex graph for clang on linux. I can't find anything wrong with my file and have succeeded compiling and running the test code with clang-4.0 on a linux computer, so I wonder if there is an issue with the compiler on the test machine. Nevertheless, I suppose this needs to be addressed.

Best wishes.

anadon · 2018-09-02T21:30:07Z

I'm in the process of dealing with some other build and test failures. When I'm done with those, I'll have you rebase your changes on upstream and that may fix your problems. Thank you for following up!

anadon · 2018-10-15T19:53:39Z

Can you rebase your PR?

mbradle · 2018-10-18T18:40:09Z

I've squashed and reworded commits. I've rebased to boostorg:develop. I'm still seeing a couple of travis-ci failures that I can't reproduce on my end. Thanks, and best wishes.

anadon · 2018-10-18T19:12:30Z

You test around line 820 has a template faux pas, and is causing timeout errors. Better, but you still need to put in just a little more work. You're close!

anadon · 2018-10-30T22:55:36Z

I'll get to a proper review when I can. This week got really busy for me, sorry!

anadon · 2018-11-01T14:12:40Z

@mbradle It appears that you're still not rebased on boostorg/graph's devel branch. Please do so.

anadon · 2018-11-05T21:01:31Z

@mbradle Looking more closely now. It looks like you've done your job and everything does appear clear. I'll need to take more time to look through though. Please be patient while I catch up!

…ion.

mbradle · 2018-11-08T02:01:04Z

Sorry, I rebased again before I saw your last message. I hope that's ok. Best wishes.

jeremy-murphy · 2018-11-08T11:00:49Z

include/boost/graph/rank_spanning_branchings.hpp

+
+      Edge e;
+
+      heap::fibonacci_heap<BranchingEntry<Edge, WeightMap, Compare> > Q;


In the same tradition of other Boost.Graph algorithms, I would recommend trying to allow the user to choose what type of property map and min heap to use, and provide these choices as defaults.
Some users will have special-case problems where different data structures are better, and in the future there might simply be a better property map, or min heap in general.

You'll have to experiment to find the best way to do it, but in general by making these data structures parameters to the algorithm so that users can pass them in by reference and an overload without those parameters that chooses the defaults. It will require some experimentation and might have to be all or nothing.

The other benefit of letting the client pass in data structures by reference is that the final result of computation is in them for the client to query afterwards if they like. (Think, for example, the open and closed sets in A*.)

I'm going to sit back and see how someone more experienced reviews these.

So a default of a Fibonacci heap, but are there any extra concepts which must be true for a data structure to be viable that can be checked for?

Thanks for this. I use two priority queues--one to keep track of branchings in the ranking routine and one to keep track of edge weights in the best and next routines. The former is just a regular priority queue (PriorityQueue concept), but the latter needs to be mergeable (MergablePriorityQueue concept). Neither one is "visible" to the user, and the former queue will have had all its entries popped off if all branchings have been found, but I do think it makes sense to allow the user to choose, with the Fibonacci heap as the default. I'll experiment with this (I think there are some examples in Boost Graph), and with the desirability of distinguishing between the two queues (as input--that is, pq1 for the branchings and pq2 for the graph edge weights).

jeremy-murphy · 2018-11-08T11:07:21Z

include/boost/graph/rank_spanning_branchings.hpp

+
+      vertex_idx_t n = num_vertices(g);
+
+      disjoint_sets<Rank, Pred> W( rank, pred1 ), S( rank, pred2 );


I understand the motivation for single-letter variable names like W, S, C, etc, but in my experience, it's better to use a descriptive name. Not everyone has the algorithm paper on hand to look up what the letters mean. And it makes the variables awfully hard to text search for in a file, such as in this review.
g for graph, u and v for vertices, etc I think is fine... there is no perfect rule about it.

Thanks, I agree with this suggestion. Going through and making the changes will probably only reinforce your point.

jeremy-murphy · 2018-11-08T11:08:21Z

include/boost/graph/rank_spanning_branchings.hpp

+
+      unordered_set<Edge> best_branching, empty_set;
+
+      shared_ptr< std::pair<Edge, weight_t> > p;


Why shared_ptr? Is that really necessary?

Why avoid a shared pointer? What is bad about them?

Like any programming construct they are not bad intrinsically, but they are often misused when a simpler construct would suffice.

Thanks for this. I used pointers to check for uninitialized or not found objects, but I think maybe boost::optional is better. For example,

Suggested change

shared_ptr< std::pair<Edge, weight_t> > p;

boost::optional< std::pair<Edge, weight_t> > p;

The NEXT routine could then return the appropriate pair, if found. If not found, p will be null (I guess really boost::none), as in the current implementation. I have a branch that does that successfully. Are there any caveats to boost::optional?

…d of pointers.

jeremy-murphy · 2018-11-21T15:07:48Z

include/boost/graph/rank_spanning_branchings.hpp

+      else
+      {
+	return;
+      }


Some of the indentation here seems to have gone awry?

jeremy-murphy · 2018-11-21T15:08:44Z

include/boost/graph/rank_spanning_branchings.hpp

+                             unordered_set<Edge>& include_edges,
+                             unordered_set<Edge>& exclude_edges
+                           )
+    {


This function, next_spanning_branching, is very long. I recommend you break it up along the lines of major sequences, loops and/or conditions.

…iography.

anadon · 2019-05-28T20:13:57Z

I have a couple of big, no change PR's you might want to keep an eye on.

…f queues in implementation routine. Rename some variables.

mbradle force-pushed the dev branch from 3106e71 to 7b9f5e4 Compare October 17, 2018 15:41

mbradle force-pushed the dev branch 3 times, most recently from ed7585b to 613a5f5 Compare October 23, 2018 21:05

anadon mentioned this pull request Nov 6, 2018

Dev3 #132

Closed

Add rank spanning branchings routine, examples, tests, and documentat…

3b95d7f

…ion.

mbradle force-pushed the dev branch from 613a5f5 to 3b95d7f Compare November 8, 2018 01:55

jeremy-murphy reviewed Nov 8, 2018

View reviewed changes

Rename cycle branching variable. Change to use boost::optional instea…

65ab91c

…d of pointers.

jeremy-murphy reviewed Nov 21, 2018

View reviewed changes

include/boost/graph/rank_spanning_branchings.hpp

else

{

return;

}

Copy link

Contributor

jeremy-murphy Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the indentation here seems to have gone awry?

jeremy-murphy reviewed Nov 21, 2018

View reviewed changes

mbradle added 6 commits May 19, 2019 14:51

Expand variable name.

2d4f9af

Alias heap.

3d9aa8a

Add comments.

c2a8730

Rename variables.

802e9de

Merge remote-tracking branch 'upstream/develop' into dev. Update bibl…

4b1f296

…iography.

Rename variables. Change zero weight for example code.

140d9bb

mbradle added 7 commits May 29, 2019 15:33

Rename disjoint sets to better indicate their purposes.

3997129

Move SEEK. Simplify templates.

2ca821e

Change to reflect change in SEEK implementation.

6d164c0

Rename new branching. Add comments.

9727258

Change name of priority queue template parameter. Put instantiation o…

4637e05

…f queues in implementation routine. Rename some variables.

Add fibonacci test temporarily for regression test.

07f0def

Simplify test.

777010b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rank spanning branchings routine for possible inclusion in library. #82

Add rank spanning branchings routine for possible inclusion in library. #82

mbradle commented Nov 6, 2016

anadon commented Aug 31, 2018

mbradle commented Sep 2, 2018

anadon commented Sep 2, 2018

anadon commented Oct 15, 2018

mbradle commented Oct 18, 2018

anadon commented Oct 18, 2018

anadon commented Oct 30, 2018

anadon commented Nov 1, 2018

anadon commented Nov 5, 2018

mbradle commented Nov 8, 2018

jeremy-murphy Nov 8, 2018

anadon Nov 8, 2018

mbradle Nov 20, 2018 •

edited

Loading

jeremy-murphy Nov 8, 2018

mbradle Nov 20, 2018

jeremy-murphy Nov 8, 2018

anadon Nov 8, 2018

jeremy-murphy Nov 19, 2018

mbradle Nov 20, 2018

jeremy-murphy Nov 21, 2018

jeremy-murphy Nov 21, 2018

anadon commented May 28, 2019


		Edge e;

		heap::fibonacci_heap<BranchingEntry<Edge, WeightMap, Compare> > Q;


		vertex_idx_t n = num_vertices(g);

		disjoint_sets<Rank, Pred> W( rank, pred1 ), S( rank, pred2 );


		unordered_set<Edge> best_branching, empty_set;

		shared_ptr< std::pair<Edge, weight_t> > p;

	shared_ptr< std::pair<Edge, weight_t> > p;
	boost::optional< std::pair<Edge, weight_t> > p;

Add rank spanning branchings routine for possible inclusion in library. #82

Are you sure you want to change the base?

Add rank spanning branchings routine for possible inclusion in library. #82

Conversation

mbradle commented Nov 6, 2016

anadon commented Aug 31, 2018

mbradle commented Sep 2, 2018

anadon commented Sep 2, 2018

anadon commented Oct 15, 2018

mbradle commented Oct 18, 2018

anadon commented Oct 18, 2018

anadon commented Oct 30, 2018

anadon commented Nov 1, 2018

anadon commented Nov 5, 2018

mbradle commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbradle Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anadon commented May 28, 2019

mbradle Nov 20, 2018 •

edited

Loading