Closed dcoudert closed 6 years ago
Hello, this is my first attempt to review a patch... feel free to let me know if any of these suggestions are inappropriate or out of scope!
First, am I correct that RandomRegularBipartite
does not sample uniformly from the set of random regular bipartite graphs with the given sizes and degrees? If this is the case, then the documentation for this function should make it more clear what the sampled distribution is.
Second, would you mind adding documentation (and possibly tests) for the position dictionaries generated by RandomBipartite
and RandomRegularBipartite
?
I also noticed that the only other random graph generator which assigns positions is RandomTriangulation
and it only does so if the parameter set_position
is explicitly set to True
. Should these two functions thus also be given a set_position
parameter for consistency?
Finally, the name given to the graph generated by RandomRegularBipartite
could be made more informative, perhaps something like:
name="Random regular bipartite graph of size {}+{} with degrees {} and {}".format(n1, n2, d1, d2)
Branch pushed to git repo; I updated commit sha1. New commits:
9ae579f | trac #25403: more comments |
Branch pushed to git repo; I updated commit sha1. New commits:
9ef139e | trac #25403: add parameter set_position |
Thank you for reviewing this ticket. It helps a lot.
I have implemented all your comments. For RandomTriangulation
, the time to compute the positions of vertices is non negligible (planar embedding). Here, it's quite cheap, but I agree that in many cases we don't need it.
Don't forget to put your real name in the field "Reviewers"
Thanks for making the changes! It seems however there is some issue with the tests. After checking out your branch and building, sage -t src/sage/graphs/generators/random.py
fails with a timeout. It seems the culprit is:
sage: graphs.RandomRegularBipartite(2, 3, 3, set_position=True).get_pos() ## line 318 ##
This particular graph must be complete bipartite, so perhaps there is some issue when generating these. In the complete bipartite case (d1 == n2
) maybe the edge generation should just be made deterministic.
Finally, it would probably be more helpful to users if the coordinates of the positions were included in the docstring in addition to the code comments.
Reviewer: Bryan Gin-ge Chen
It's more complicated than that. If found other situations like graphs.RandomRegularBipartite(6, 6, 5, set_position=True)
that can loop endlessly :(
So we need another, more secured, algorithms...
Branch pushed to git repo; I updated commit sha1. New commits:
8000a65 | trac #25403: improved algorithm |
This new version should perform better.
Branch pushed to git repo; I updated commit sha1. New commits:
87ea629 | various changes and fixes |
Hi, I went ahead and pushed some changes to this branch:
retart
instead of restart
.sage.graphs.CompleteBipartiteGraph
. That function places the two parts horizontally and also uses differently scaled spacing.Everything else seemed OK, so you may set to positive_review if you're happy with my changes.
New commits:
87ea629 | various changes and fixes |
Thank you for all the corrections.
For the consistency of positions, what I can do is add a method in graph_plot.py
for bipartite embedding, like _bipartite_embedding(g, V1, V2, first=(0,1), last=(1,0))
in the flavor of _line_embedding
, and then use it in all bipartite graphs generators.
It can proceed as the plotting of CompleteBipartiteGraph
.
Would you agree on that ?
Replying to @dcoudert:
For the consistency of positions, what I can do is add a method in
graph_plot.py
for bipartite embedding, like_bipartite_embedding(g, V1, V2, first=(0,1), last=(1,0))
in the flavor of_line_embedding
, and then use it in all bipartite graphs generators. It can proceed as the plotting ofCompleteBipartiteGraph
. Would you agree on that ?
I wasn't familiar with those methods in graph_plot
, but this sounds like a great solution! We should probably update the ticket title / description now though.
Description changed:
---
+++
@@ -1,2 +1,3 @@
-Add a generator of random regular bipartite graph.
+Add a generator of random regular bipartite graph, `RandomRegularBipartite`.
+This ticket also moves the graph embedding method in `CompleteBipartite` to `graph_plot` so that embeddings for `CompleteBipartiteGraph`, `RandomRegularBipartite` and `RandomBipartite` can be easily made consistent.
Description changed:
---
+++
@@ -1,3 +1,3 @@
Add a generator of random regular bipartite graph, `RandomRegularBipartite`.
-This ticket also moves the graph embedding method in `CompleteBipartite` to `graph_plot` so that embeddings for `CompleteBipartiteGraph`, `RandomRegularBipartite` and `RandomBipartite` can be easily made consistent.
+This ticket also unifies the graph embedding method of `CompleteBipartiteGraph`, `RandomRegularBipartite` and `RandomBipartite` by using method `_line_embedding` from `graph_plot.py`.
I have chosen not to create a new method in graph_plot.py
as the need for bipartite embedding is rather limited so far. I use 2 calls to _line_embedding
instead.
I have corrected the method for _line_embedding
to avoid errors with graphs on 0 or 1 vertex.
Branch pushed to git repo; I updated commit sha1. New commits:
315feb7 | Remove trailing whitespace |
Branch pushed to git repo; I updated commit sha1. New commits:
5c7a97d | Fix doctest in `sage/graphs/line_graph.py` |
I removed a trailing whitespace and fixed a doctest that was broken by the change to graph name inside CompleteBipartiteGraph
. Feel free to set to positive_review after you see this.
So then. Thanks.
Changed branch from public/25403_random_regular_bipartite to 5c7a97d
Add a generator of random regular bipartite graph,
RandomRegularBipartite
.This ticket also unifies the graph embedding method of
CompleteBipartiteGraph
,RandomRegularBipartite
andRandomBipartite
by using method_line_embedding
fromgraph_plot.py
.Component: graph theory
Author: David Coudert
Branch/Commit:
5c7a97d
Reviewer: Bryan Gin-ge Chen
Issue created by migration from https://trac.sagemath.org/ticket/25403