Branchings.py - Removed graphs and branchings structures in favor of an unique for unrolling the circuits #2117

Loveuse · 2016-05-03T13:05:47Z

Removed the structures that contain the graphs and branchings of all levels and inserted an unique structure, called unroll, that stores the edges involved in a circuits for all levels, in order to rebuild the graph at every level in the second phase of the algorithm.

…levels and inserted an unique structure, called unroll, that stores the edges involved in a circuits for all levels, in order to rebuild the graph at every level.

chebee7i · 2016-05-04T00:19:10Z

On a quick first look, it seems that you are basing this off an older version of NetworkX. The reason I say this is that edge_subgraph was removed from the current module and it looks like you've added it back in. It also shows you adding back functions like is_tree and is_arborescence. These should no longer be in the branchings.py module since we have them available in the recognition.py module. Could you confirm whether or not my initial synopsis is correct?

chebee7i · 2016-05-04T00:21:54Z

Current version is here: https://github.com/networkx/networkx/blob/master/networkx/algorithms/tree/branchings.py

Loveuse · 2016-05-04T09:59:40Z

Thank you for giving it a look! I can confirm your synopsis. I am going to fix it.

chebee7i · 2016-05-04T13:53:20Z

OK looks much more how I expected. This is really nice to see, and something we needed but never had time to do. A couple quick questions for you:

While implementing this, and also for pedagogical reasons, it can be useful to see the intermediate structures. Are we still able to build them at each iteration? If not, could we add a new method that builds them for you?
When I originally implemented this, I had also considered the more general weighted matroid intersection problem. Have you looked at this problem from this angle, and if so, do you think it might eventually be worthwhile to implement it? (Not for this pull request of course. There's also a number of other speed improvements in the references at the top of the module that we could look into implementing one day.)

chebee7i · 2016-05-04T13:54:09Z

I need to give this a better look when I get off work today. Look for a merge later tonight.

jfinkels · 2016-05-04T14:22:52Z

networkx/algorithms/tree/branchings.py

-                if self.store:
-                    self.graphs.append(G.copy())
-                    self.branchings.append(B.copy())
+        def getFinalGraphAndBranch(nodes):


Python functions should be underscore_separated not CamelCase.

Loveuse · 2016-05-04T20:02:08Z

I am glad for you words @chebee7i. To answer your questions:

I didn't think about it, because for my purposes I only need of less memory usage. We would still able to do that by storing the results of the first phase of the algorithm (graph and branching) and from them we could build the graphs and branchings of each iteration, in the same way of second phase of the algorithm. I don't know how to proceed on this. Would it be better have different methods for the two versions (with intermediate results and not)? In this case, I would suggest to add a method in order to rebuilding the intermediate results. Or have you got other ideas?
I have never looked at this problem! But I am going to take a look on it in the next days, in any case we can contact each other for discussing about it!

I want to thank @jfinkels about the comments. I am going to follow them.

…Removed print comments and fixed other part of code as directed me.

chebee7i · 2016-05-05T03:04:50Z

networkx/algorithms/tree/branchings.py

+                    #   (b) every node of G^i is in D^i and E^i is a branching
+                    # Construction guarantees that it's a branching.
+                    if len(G) != len(B):
+                        raise Exception('Graph and branching must be of the same lenght.')


Small typo here.

Where is it?

Opps: must be of the same lenght

I don't really see the problem! :)

The word is "length". However, a better message would be "graph and branching must have the same number of nodes", since a graph doesn't really have a "length".

How could I have been so blind! I am going to change the message.

chebee7i · 2016-05-05T03:42:41Z

This looks good to me. As to point 1) above, the memory usage is a definite improvement, so we need to keep that. It might make sense to refactor the find_optimium() method a bit more. For example, should the stuff from

# (I3) Branch construction.

onward be moved into its own method? This would separate the first phase from the second phase of the algorithm. Then, as you suggested, the first part of the algorithm could take an extra parameter that instructs whether or not we should keep the information to build the intermediate structures.

I'll leave it up to you whether this should be implemented at all, and if so, precisely how to implement it. At this point, you are much closer to the code than I am now. There's more than a bit of effort involved in getting one's state of mind back into Edmond's. Reading the paper, seeing how the algorithm progresses, and comparing to the pictures in the paper was my main motivation for keeping the intermediate structures. My goal was to imagine how easy/difficult it would be for someone to come back to this code many years from now after not having touched/read it...which is my current situation :). So factor that into your decision. Either decision is fine, since the original code will always be available in git anyway. Honestly, this code/algorithm is a bit esoteric in the sense that very few people have needed it...certainly the two of us, and probably only a handful of other people. Let us know what you decide.

Loveuse · 2016-05-05T10:14:20Z

I totally agree on you considerations! In the same way of the find_optimium() method, the methods that invoke it (maximum_spanning_arborescence, etc.) should have the same parameter to indicate whether or not we should keep the information to build the intermediate structures.
Furthermore I take this opportunity to thank you for having implemented it, and also for giving me the freedom of decision on it.
Anyway I am going to implement this modifications.

chebee7i · 2016-05-11T02:39:29Z

Everything going well on this? No prob if things are busy.

Loveuse · 2016-05-11T09:31:54Z

I am sorry, I am very busy recently. However I was thinking about how to implement it, and doing it as we said, we will spend cpu time to rebuild the structures in the second phase and more memory usage due the necessary structures for this purpose . So I am thinking to mix the two versions and making them coexist using the parameter. What do you think about it?
Furthermore, I'm evaluating the possibility of implementing the algorithm from the paper "The k best spanning arborescences of a network" of P. M. Camerini, L. Fratta, F. Maffioli.

chebee7i · 2016-05-16T20:09:33Z

Yeah that sounds good. The k-best spanning arborescences sounds neat---definitely a separate pull request.

Loveuse · 2016-05-17T10:41:19Z

Ok, I am going to push the code for this pull request soon.
In regards to my work, I have to implement only the first algorithm of the k-best spanning arborescence (best), in order to retrieve the best spanning arborescence given/without a root or the best branching. I don't know if I have enough time to implement it entirely.

…f intermediate results.

…2883) Removed not_implemented_for decorator from line_graph function. See networkx#2814 and networkx#2880.

…#2871) * Add greedy modularity maximization community detection. * Adds modularity_max module to networkx.algorithms.community package * Adds greedy_modularity_communities() function * Adds test using Zachary karate club network. * Add Clauset-Newman-Moore community detection. * Replace greedy_modularity_communities() with CNM implementation. * Add networkx.utils.mapped_queue used by the above implementation. * Add tests for networkx.utils.mapped_queue. * Add tests for naive modularity maximization. * Add TestNaive class. * Remove redundant modularity calculation. * Removes call to modularity() left over from debugging. Results in a significant speed-up. * Comply with pep8. * Update modularity_max module and tests for pep8. * Update mapped_queue module and tests for pep8. * Fix import of MappedQueue. * Add documentation for modularity_max module.

* reconstruct_path source code * minor update to examples * test_reconstruct_path * updated example * fixed tests and __all__ declaration * updated docstring example * typo

* Updated release notes. Updated release notes for topological sort changes * Updated migration guide. Updated migration guide for changes to topological sort. Changed "iterator" to "iterable" where appropriate. * Update migration_guide_from_1.x_to_2.0.rst Reverted iterator -> iterable. * Update migration_guide_from_1.x_to_2.0.rst Fixed missing parentheses. * Update migration_guide_from_1.x_to_2.0.rst

…levels and inserted an unique structure, called unroll, that stores the edges involved in a circuits for all levels, in order to rebuild the graph at every level.

…Removed print comments and fixed other part of code as directed me.

…f intermediate results.

Removed the structures that contain the graphs and branchings of all …

25bef79

…levels and inserted an unique structure, called unroll, that stores the edges involved in a circuits for all levels, in order to rebuild the graph at every level.

chebee7i mentioned this pull request May 4, 2016

Branchings.py - A new approach #2115

Closed

Fixed problems related to networkx file version

c737ce0

jfinkels reviewed May 4, 2016
View reviewed changes

Loveuse added 2 commits May 4, 2016 22:57

Used exceptions instead asserts (looking for appropriated messages). …

154cebc

…Removed print comments and fixed other part of code as directed me.

Fixed bugs on the edges properties of the unroll structure.

b91cec0

chebee7i reviewed May 5, 2016
View reviewed changes

Fixed exception message.

131cae6

Loveuse added 4 commits May 17, 2016 13:12

Made to coexist the two versions of code that differ on the storing o…

0dec15f

…f intermediate results.

Fixed parameters passed to the functions

a870561

Added a missing character

ee3fdd1

Fixed bug.

a08b366

Changed variable in favor of more appropriate one

df02e44

hagberg added this to the networkx-future milestone Jun 15, 2016

Loveuse and others added 13 commits February 19, 2018 15:11

Merge branch 'master' into refactoring-branchings

2fc1c8c

Fix networkx#2880: Allow line_graph to apply to multigraph (networkx#…

5e7606f

…2883) Removed not_implemented_for decorator from line_graph function. See networkx#2814 and networkx#2880.

link DOIs to preferred resolver (networkx#2879)

d29207c

Floyd Warshall rebuild path (networkx#2870)

9769741

* reconstruct_path source code * minor update to examples * test_reconstruct_path * updated example * fixed tests and __all__ declaration * updated docstring example * typo

Removed the structures that contain the graphs and branchings of all …

0be0bc4

…levels and inserted an unique structure, called unroll, that stores the edges involved in a circuits for all levels, in order to rebuild the graph at every level.

Fixed problems related to networkx file version

0735136

Used exceptions instead asserts (looking for appropriated messages). …

39b012f

…Removed print comments and fixed other part of code as directed me.

Fixed bugs on the edges properties of the unroll structure.

29ac692

Made to coexist the two versions of code that differ on the storing o…

d00ce3a

…f intermediate results.

Fixed parameters passed to the functions

3d8f58a

Fixed bug.

9786a13

leamingrad mentioned this pull request May 6, 2018

Preserve edge attributes when using branching algorithms #2965

Merged

Base automatically changed from master to main March 4, 2021 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branchings.py - Removed graphs and branchings structures in favor of an unique for unrolling the circuits #2117

Branchings.py - Removed graphs and branchings structures in favor of an unique for unrolling the circuits #2117

Loveuse commented May 3, 2016

chebee7i commented May 4, 2016 •

edited

Loading

chebee7i commented May 4, 2016

Loveuse commented May 4, 2016

chebee7i commented May 4, 2016

chebee7i commented May 4, 2016

jfinkels May 4, 2016

Loveuse commented May 4, 2016

chebee7i May 5, 2016

Loveuse May 5, 2016

chebee7i May 5, 2016

Loveuse May 5, 2016

jfinkels May 5, 2016

Loveuse May 5, 2016

chebee7i commented May 5, 2016

Loveuse commented May 5, 2016

chebee7i commented May 11, 2016

Loveuse commented May 11, 2016

chebee7i commented May 16, 2016

Loveuse commented May 17, 2016

Branchings.py - Removed graphs and branchings structures in favor of an unique for unrolling the circuits #2117

Are you sure you want to change the base?

Branchings.py - Removed graphs and branchings structures in favor of an unique for unrolling the circuits #2117

Conversation

Loveuse commented May 3, 2016

chebee7i commented May 4, 2016 • edited Loading

chebee7i commented May 4, 2016

Loveuse commented May 4, 2016

chebee7i commented May 4, 2016

chebee7i commented May 4, 2016

jfinkels May 4, 2016

Choose a reason for hiding this comment

Loveuse commented May 4, 2016

chebee7i May 5, 2016

Choose a reason for hiding this comment

Loveuse May 5, 2016

Choose a reason for hiding this comment

chebee7i May 5, 2016

Choose a reason for hiding this comment

Loveuse May 5, 2016

Choose a reason for hiding this comment

jfinkels May 5, 2016

Choose a reason for hiding this comment

Loveuse May 5, 2016

Choose a reason for hiding this comment

chebee7i commented May 5, 2016

Loveuse commented May 5, 2016

chebee7i commented May 11, 2016

Loveuse commented May 11, 2016

chebee7i commented May 16, 2016

Loveuse commented May 17, 2016

chebee7i commented May 4, 2016 •

edited

Loading