Skip to content
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

Graph.spanning_trees does not like loops #19291

Closed
sagetrac-Stefan mannequin opened this issue Sep 25, 2015 · 9 comments
Closed

Graph.spanning_trees does not like loops #19291

sagetrac-Stefan mannequin opened this issue Sep 25, 2015 · 9 comments

Comments

@sagetrac-Stefan
Copy link
Mannequin

sagetrac-Stefan mannequin commented Sep 25, 2015

G = Graph({0:{0:'a', 1:'b'}, 1:{2: 'c'}, 2:{0: 'e'}}, loops=True)
S = G.spanning_trees()

goes "boom" (recursion depth exceeded). It also destroys G in the process, which shouldn't happen, I think.

Component: graph theory

Author: Nathann Cohen

Branch/Commit: 6a86e23

Reviewer: Stefan van Zwam

Issue created by migration from https://trac.sagemath.org/ticket/19291

@sagetrac-Stefan sagetrac-Stefan mannequin added this to the sage-6.9 milestone Sep 25, 2015
@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 25, 2015

Branch: u/ncohen/19291

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 25, 2015

comment:1

Here is a fix. By the way, if you need this function for some hard computations know that there is room for much improvement in there.

Nathann


New commits:

6a86e23trac #19291: Graph.spanning_trees does not like loops

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 25, 2015

Commit: 6a86e23

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 25, 2015

Author: Nathann Cohen

@nathanncohen nathanncohen mannequin added the s: needs review label Sep 25, 2015
@sagetrac-Stefan
Copy link
Mannequin Author

sagetrac-Stefan mannequin commented Sep 25, 2015

Reviewer: Stefan van Zwam

@sagetrac-Stefan
Copy link
Mannequin Author

sagetrac-Stefan mannequin commented Sep 25, 2015

comment:2

I was just exploring some graph methods, I don't actually need to use it for now.

Your change forces a copy of the graph to be made regardless of whether there are loops. That's definitely safer than what happened before, but could be costly in terms of memory for big graphs. Still, I prefer the corresponding increase in safety of the code, so I will approve.

@dcoudert
Copy link
Contributor

comment:3

Replying to @sagetrac-Stefan:

but could be costly in terms of memory for big graphs.

Well, considering such running time, it is likely that no one will use this method for "big graphs".

sage: g = graphs.GridGraph([2,2,2])
sage: %timeit g.spanning_trees()
10 loops, best of 3: 99.4 ms per loop
sage: g = graphs.GridGraph([2,2,3])
sage: %timeit g.spanning_trees()
1 loops, best of 3: 13.4 s per loop
sage: g.order(),g.size()
(12, 20)

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 26, 2015

comment:4

Yeah, precisely. Stefan: to answer your question, I did not mind adding a graph copy to this method because, well, given the way it is written, the amount of wasted time is so high that this copy makes no difference. If you ever need this method "seriously", it will have to be reimplemented.

Nathann

@vbraun
Copy link
Member

vbraun commented Oct 12, 2015

Changed branch from u/ncohen/19291 to 6a86e23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants