-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Two new algorithms for cycles and extension of cycle_basis #1067
Conversation
I think an example demonstrating how to work with a given circuit could go into examples/algorithms. |
here are some questions... feel free to answer "yes, I think this is best". I'm not saying it isn't, I'm just asking. Does chords() require finding a spanning tree or a minimum spanning tree? Which would be better? There are many e[:2] and e[:2][::-1] used in cycle_basis_matrix. I think I see why -- to handle multigraphs which have 3-tuples for edges. The code returns the node-tuple or its reverse. Would it be cleaner to split "e" into the nodes, or does that require more convolutions? You had some tests at the end of your original script. Did all those tests get transferred over to test_cycles? In the docs for chords() you should specify that the output objects are networkx graphs. Is that what you want or would it be better to return sets of edges? Looks good! Thanks, |
Does chords() require finding a spanning tree or a minimum spanning tree? Which would be better? There are many e[:2] and e[:2][::-1] used in cycle_basis_matrix. I think I see why -- to handle multigraphs which have 3-tuples for edges. The code returns the node-tuple or its reverse. Would it be cleaner to split "e" into the nodes, or does that require more convolutions? You had some tests at the end of your original script. Did all those tests get transferred over to test_cycles? In the docs for chords() you should specify that the output objects are networkx graphs. Is that what you want or would it be better to return sets of edges? |
I believe the pull request is just a pointer to your repository. If you It is also true that others cannot change the pull request directly. We My question about the e[:2][::-1] notation is this: Could this be done Dan On Sun, Mar 2, 2014 at 4:31 PM, Juan Pablo Carbajal <
|
…cles_basis_matrix.
…ata. --HG-- extra : amend_source : 866177d7c67f829d992279ee1dd474772a452bb0
I have improved chords, added tests and used names of the edge and reversed edged as suggested by Dan. Regarding spanning trees. I think I used minimum_spanning_tree because I could not find the algorithm to calculate any one spanning tree. Is there an efficiency overhead due to minimum_spanning_tree? Is there a better algorithm to get just one of the spanning trees? |
I have added an example to the example/algorithms folder. I hope now the merge can be executed. Let me know if there is anything missing. |
Pinging this request. Is there anything I can do to facilitate the merge? Thanks |
To get a single spanning tree, you can just run a BFS. To sample a spanning tree according to its weight, there is a really nice algorithm [1]. You can read the paper here. I might implement it in the near future myself. [1] Wilson, David Bruce. "Generating random spanning trees more quickly than the cover time." Proceedings of the twenty-eighth annual ACM symposium on Theory of computing. ACM, 1996. |
Thank you for your answer. |
The tests are still failing. Fixing those is necessary to do the merge. I think the rest needs a careful review and likely some adjustments. I'd also like to see scipy sparse matrices used. That is what we are now using (or intend to change) in the rest of the NetworkX package. |
The failing tests seem to be related to older versions of python. Who can one check the offending commands? |
Regarding, sparse matrices. I will implement this in the future, it is marked as a todo in the code. Is this a reason not to merge the code? |
The scipy import needs protection from when the module isn't found (scipy is not required). |
Well, the future is now! So might as well get it implemented with scipy.sparse matrices so we don't have to go and fix it later. |
Assuming you have the time, yes. I will see what I can do. Though I would ask you to merge this and then improve. Maybe there are useful reports if people use this code. |
Apparently the problem with Travis is that the tests are importing scipy to compare the results. |
Ok, Travis is happy now. |
What is the status of this PR? |
It looks like it could use a bit more editing. But I suspect that duties took the author elsewhere, so we might have to tackle them. |
At the moment I am in holidays. On Wed, Aug 6, 2014 at 12:34 AM, chebee7i notifications@github.com wrote:
|
I can take a look at this. |
Some comments.
I'm about to open a new pull request that makes some style and documentation fixes, and partially implements item 3. Some tests are still missing. |
See #1538 |
I have merged my algorithms with networkx. I tried to follow coding standards by looking at other files.
I have added tests.
I would like to know how to create demos to show the use of cycle_basis and cycle_basis_matrix to generate Kirchhof equations for a given circuit (directed multigraph).
Thanks