Sage: Ticket #6828: [with patch, positive review] Random Bipartite Graph
https://trac.sagemath.org/ticket/6828
<p>
I needed to create one of those, and thought it may be useful in sage... Several lines for a new Graph generator ;-)
</p>
<p>
I used the position dictionary from <a class="missing wiki">CompleteBipartiteGraph?</a>, so we should be out of trouble on this point ;-)
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/6828
Trac 1.1.6ncohenWed, 26 Aug 2009 13:36:29 GMTattachment set
https://trac.sagemath.org/ticket/6828
https://trac.sagemath.org/ticket/6828
<ul>
<li><strong>attachment</strong>
set to <em>random_bipartite.patch</em>
</li>
</ul>
TicketrbeezerTue, 22 Sep 2009 06:21:16 GMTsummary changed
https://trac.sagemath.org/ticket/6828#comment:1
https://trac.sagemath.org/ticket/6828#comment:1
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] Random Bipartite Graph</em> to <em>[with patch, needs work] Random Bipartite Graph</em>
</li>
</ul>
<p>
Hi Nathann,
</p>
<p>
Several of my comments at <a class="closed ticket" href="https://trac.sagemath.org/ticket/6823" title="enhancement: [with patch, positive review] Kneser Graph in graph_generators (closed: fixed)">#6823</a> apply here - just a diff file rather than a Mercurial file, use "trac_xxxx" for the filename, and the inputs should be checked for errors. In this case the probability should be checked and n1 and n2 should be checked that they are positive - making one negative does decrease the total.
</p>
<p>
It would seem that the final call to the <code>BipartiteGraph</code> generator can cause problems. Try 15 to 20 vertices in each part with a very low edge-probability like 0.01, then despite having a good pos_dict, the isolated vertices of one part move into a different "half" of the plot.
</p>
<p>
Can you make the name look better than <code>Random bipartite graph: graph on 30 vertices</code>? Maybe this is a consequence of final call as well. Maybe the probability could be included in the name?
</p>
<p>
I think the construction <code>range(n1+n2)[n1:]</code> can be accomplished more clearly with <code>range(n1,n1+n2)</code>.
</p>
<p>
Rob
</p>
TicketncohenSat, 26 Sep 2009 15:26:20 GMTsummary changed
https://trac.sagemath.org/ticket/6828#comment:2
https://trac.sagemath.org/ticket/6828#comment:2
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs work] Random Bipartite Graph</em> to <em>[with patch, needs review] Random Bipartite Graph</em>
</li>
</ul>
<p>
Looks like I went into a lot of trouble for nothing ;-)
</p>
<p>
I did not even need to create the positions myself !
</p>
<p>
Thanks for your remarks, I am slowly learning Python through reviews :-)
</p>
<p>
Nathann
</p>
TicketrbeezerSun, 27 Sep 2009 06:25:09 GMT
https://trac.sagemath.org/ticket/6828#comment:3
https://trac.sagemath.org/ticket/6828#comment:3
<p>
Hi Nathann,
</p>
<p>
I like the new names for the vertices, and it looks much better for graphs with low edge probability the way you are doing it now. Some more comments:
</p>
<ol><li> Checking for errors usually involves raising an error, rather than using an assert. Poke around in the code and I think you will see more often a style like:
</li></ol><pre class="wiki">if not((p>=0 and p<=1):
raise ValueError, "Parameter p is a probability, and so should be a real value between 0 and 1"
</pre><p>
I'd place them after the imports, but that's just me.
</p>
<ol start="2"><li> You need to paste in the output of your test. Right now it is failing. You really should make sure all your tests pass before someone stops in to do a review.
</li></ol><ol start="3"><li> You should also test the two checks on the input - in a section called <code>TESTS</code>
</li></ol><ol start="4"><li> Your EXAMPLE section needs to have two semicolons to create the verbatim section, so at least you need a <code>::</code> on a line by itself prior to each test. (Maybe two semicolons right after EXAMPLE will work as well - I'm having trouble checking this right now with my setup.) This ensures you get the right formatting in the reference manual. Again, take a look at what is done elsewhere in the source and compare with the output, then make sure your output also works properly before seeking a review.
</li></ol><p>
I think most of the above applies to <a class="closed ticket" href="https://trac.sagemath.org/ticket/6823" title="enhancement: [with patch, positive review] Kneser Graph in graph_generators (closed: fixed)">#6823</a>, which I haven't commented on. Adding the Odd graphs there is great, though.
</p>
<p>
Rob
</p>
TicketrbeezerSun, 27 Sep 2009 06:33:34 GMTsummary changed
https://trac.sagemath.org/ticket/6828#comment:4
https://trac.sagemath.org/ticket/6828#comment:4
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] Random Bipartite Graph</em> to <em>[with patch, needs work] Random Bipartite Graph</em>
</li>
</ul>
TicketncohenTue, 29 Sep 2009 09:51:13 GMTsummary changed
https://trac.sagemath.org/ticket/6828#comment:5
https://trac.sagemath.org/ticket/6828#comment:5
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs work] Random Bipartite Graph</em> to <em>[with patch, needs review] Random Bipartite Graph</em>
</li>
</ul>
<p>
This one should be Ok, sorry for the trouble... ;-)
</p>
<p>
By the way, I built the documentation and was able to see a TESTS:: section.. I thought this part would be hided in the documentation as it is meant to be read by users and "tests" is something like an "internal tool".
</p>
<p>
I thought I'd put the "raise" after the "import", because in case the inputs are wrong there is no need to import libraries.. <sup></sup>;
</p>
<p>
Nathann
</p>
TicketncohenTue, 29 Sep 2009 09:51:26 GMTattachment set
https://trac.sagemath.org/ticket/6828
https://trac.sagemath.org/ticket/6828
<ul>
<li><strong>attachment</strong>
set to <em>trac_6828.patch</em>
</li>
</ul>
TicketrbeezerWed, 30 Sep 2009 05:25:11 GMTsummary changed; keywords, reviewer, author set
https://trac.sagemath.org/ticket/6828#comment:6
https://trac.sagemath.org/ticket/6828#comment:6
<ul>
<li><strong>keywords</strong>
<em>bipartite</em> <em>graph</em> <em>generator</em> added
</li>
<li><strong>reviewer</strong>
set to <em>Rob Beezer</em>
</li>
<li><strong>author</strong>
set to <em>Nathann Cohen</em>
</li>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] Random Bipartite Graph</em> to <em>[with patch, positive review] Random Bipartite Graph</em>
</li>
</ul>
<p>
Nathann,
</p>
<p>
Looks real good. Yes, you would think the TESTS section would not migrate to the reference manual, but right now I think it is really treated no differently. But I guess in some cases what it contains is useful to read. Maybe.
</p>
<p>
All three new graph generators are welcome additions - thanks for adding them. And thanks for your patience with all the suggestions.
</p>
<p>
Builds on 4.1.2.alpha2, passes all long tests. Positive review.
</p>
<p>
Rob
</p>
TicketmhansenThu, 15 Oct 2009 10:11:12 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/6828#comment:7
https://trac.sagemath.org/ticket/6828#comment:7
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.2.alpha0</em>
</li>
</ul>
Ticket