#10530: De Bruijn Sequence construction for combinat
---------------------------------+------------------------------------------
Reporter: eviatarbach | Owner: eviatarbach
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.7
Component: combinatorics | Keywords:
Work_issues: | Upstream: N/A
Reviewer: Nicolas M. Thiéry | Author: Eviatar Bach
Merged: | Dependencies:
---------------------------------+------------------------------------------
Changes (by ncohen):
* status: needs_review => needs_work
Comment:
Hello !
Several superficial remarks :
* As in your other patch, the lines should be kept to less than 80
characters
* Some functions are not documented
{{{
/home/ncohen/sage/devel/sage-1/sage/combinat$ sage -coverage
debruijn_sequence*
----------------------------------------------------------------------
debruijn_sequence.pyx
SCORE debruijn_sequence.pyx: 0% (0 of 2)
Missing documentation:
* gen(int t, int p):
Missing doctests:
* debruijn_sequence(int k, int n):
----------------------------------------------------------------------
----------------------------------------------------------------------
debruijn_sequences.py
ERROR: Please add a `TestSuite(s).run()` doctest.
SCORE debruijn_sequences.py: 75% (3 of 4)
Missing documentation:
* __init__(self, k,n):
----------------------------------------------------------------------
/home/ncohen/sage/devel/sage-1/sage/combinat$ sage -coverage
debruijn_sequence.pyx
----------------------------------------------------------------------
debruijn_sequence.pyx
SCORE debruijn_sequence.pyx: 0% (0 of 2)
Missing documentation:
* gen(int t, int p):
Missing doctests:
* debruijn_sequence(int k, int n):
----------------------------------------------------------------------
}}}
Let's keep this to 100% lest we write other patches later to
improve it `:-)`
* The "NOTE::" environment is misused. Look at other places in the
source code where it is used. You can see it doesn't display properly when
building the documentation
{{{
sage -docbuild reference html
}}}
* Same for the "reference" environment and citations.The best is to
look at other places in the souce code `:-)`
* The "cardinality" method, for instance, is not doctested.
* It would be good to add a reference to the book "Combinatorial
Generation" in the file containing the actual implementaion of the
algorithm
* There are 4 patches on this ticket.
* If the ticket's name is a number, let it be the ticket's number
* In this kind of situation, it's also very nice to produce
"flattened" patched. Sometimes it is better to update the current patch
than to create another one to apply on top. You may need to use "Mercurial
Queue" if you're not used to it.
Short of all this, thank you for this patch ! As usual, there's much more
work in dealing with the programming standards than with the actual math
content, but it quickly becomes natural. Thank you very much also, because
of you I spend yesterday reading random parts of Combinatorial Generation,
and learned very nice things `:-)`
(send me an email if I can be of any help)
Nathann
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/10530#comment:15>
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.
