#9676: Random Tree constructor for graphs section
----------------------------------+-----------------------------------------
Reporter: edward.scheinerman | Owner: jason, ncohen, rlm
Type: enhancement | Status: needs_work
Priority: major | Milestone:
Component: graph theory | Keywords:
Author: Ed Scheinerman | Upstream: N/A
Reviewer: | Merged:
Work_issues: |
----------------------------------+-----------------------------------------
Changes (by ncohen):
* status: new => needs_work
Comment:
Hellooooo !!
Well, a long list of comments, which is perfectly normal for a first patch
:-)
* First of all, you forgot to set our ticket as "needs_review". It means
that if you hadn't given me its number by email, I would never had noticed
this ticket, as I usually list those who are waiting to be reviewed
* I am really sorry, but I totally forgot to tell you about a patch which
was reviewed a few days ago #9373. Among other things, it removes one of
the two lists of generators given in graph_generators (where you added
your constructor), and the other one is reformatted. This means that your
patch can not be applied once #9373 has been... Hence, your patch needs to
be rebased on top of this one. (It means you first need to create a new
branch, then apply #9373, then write your patch on top of it).
* When writing docstrins, you can indeed use LaTeX formulas. For this,
you need to add a special character, so that Sphinx notices you are indeed
using LaTeX. In your case,
{{{
n^(n-2) and {0,1,...,n-1}
}}}
should become
{{{
`n^(n-2)` and `\{0,1,...,n-1\}`
}}}
* If you build the documentation after applying your patch ( sage
-docbuild reference hmtl ), you will also notice that your example does
not appear like the other ones. This is because we make use of the double-
column "::". You can give these other methods a look to see how it works,
and build the documentation to mak sure it does.
* The result you mention is, if I remember correctly, for Labelled trees.
It means that trees with a large automorphism group will appear more
often, so I think this method should be renamed RandomLabelledTree.
* I see how you are building those trees, but I do not know how to prove
this is indeed a uniform sampling. Could you add a reference for this
(even a textbook, if it is a result I should know ?). You can even add
this reference in the documentation (look for occurences of the string
REFERENCES in the code to see how it works -- as usual, uild the
documentation to ensure it is correctly understood by Sphinx).
* If you need a random permutation of [0..(n-1)], you should use
something like Permutations(n).random_element()
* In Sage's documentation, the examples you see are not just examples.
They are used to ensure that the code remains correct. This is what
happens when you write something like {{{ sage -t graph_generators.py }}
Sage reads all the docstring, and ensure what they output is what it
expects. If it is not the case, the error is reported and we know
something is wrong. Your doctest only prints it, which is not useful..
Here is one I could write, but it's really up to you, if you find
something more interesting, funnier, or can check a known result using
your function...
{{{
Checking the generated graphs are indeed trees::
sage: all( graphs.RandomTree(10).is_tree() for i in range(30) )
True
}}}
It does not need to be complicatd, most of the time. It is just a way
for Sage to check it "seems" fine, so that we notice something has gone
wrong is we modify the method is_tree or anything related.
And I am sure there is something else I had to say but forgot it O_o
Well, if you have any question, I'll be behind my emails `:-)`
Nathann
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9676#comment:1>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
