#12930: Poset of Alternating sign matrices
-------------------------------------------------------------+--------------
Reporter: aschilling | Owner:
sage-combinat
Type: enhancement | Status:
needs_review
Priority: major | Milestone:
sage-5.1
Component: combinatorics | Resolution:
Keywords: alternating sign matrices, posets, days38 | Work issues:
Report Upstream: N/A | Reviewers:
Authors: Pierre Cagne | Merged in:
Dependencies: | Stopgaps:
-------------------------------------------------------------+--------------
Comment (by aschilling):
Hi Pierre,
Here are some first comments on your patch:
- The doctest did not pass for me on /combinat/alternating_sign_matrix.py
{{{
sage -t "devel/sage-
combinat/sage/combinat/alternating_sign_matrix.py"
Exception raised by doctesting framework. Use -verbose for details.
[14.0 s]
----------------------------------------------------------------------
The following tests failed:
sage -t "devel/sage-
combinat/sage/combinat/alternating_sign_matrix.py" # Exception from
doctest framework
Total time for all tests: 14.0 seconds
d099:combinat anne$ sage -t -verbose alternating_sign_matrix.py
sage -t -verbose "devel/sage-
combinat/sage/combinat/alternating_sign_matrix.py"
Traceback (most recent call last):
File "/Users/anne/.sage//tmp/alternating_sign_matrix_49336.py", line
805, in <module>
runner=runner)
File "/Applications/sage-5.0/local/bin/sagedoctest.py", line 54, in
testmod_returning_runner
runner=runner)
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 1819, in
testmod_returning_runner
for test in finder.find(m, name, globs=globs, extraglobs=extraglobs):
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 839, in find
self._find(tests, obj, name, module, source_lines, globs, {})
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 893, in
_find
globs, seen)
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 881, in
_find
test = self._get_test(obj, name, module, globs, source_lines)
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 965, in
_get_test
filename, lineno)
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 594, in
get_doctest
return DocTest(self.get_examples(string, name), globs,
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 608, in
get_examples
return [x for x in self.parse(string, name)
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 570, in
parse
self._parse_example(m, name, lineno)
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 628, in
_parse_example
self._check_prompt_blank(source_lines, indent, name, lineno)
File "/Applications/sage-5.0/local/bin/ncadoctest.py", line 715, in
_check_prompt_blank
line[indent:indent+3], line))
ValueError: line 13 of the docstring for __main__.example_18 lacks blank
after ...: ' ....:'
Exception raised by doctesting framework. Use -verbose for details.
[1.4 s]
----------------------------------------------------------------------
The following tests failed:
sage -t -verbose "devel/sage-
combinat/sage/combinat/alternating_sign_matrix.py" # Exception from
doctest framework
Total time for all tests: 1.4 seconds
}}}
- You need doctests for all methods!!!
{{{
sage -coverage alternating_sign_matrix.py
----------------------------------------------------------------------
alternating_sign_matrix.py
SCORE alternating_sign_matrix.py: 69% (25 of 36)
Missing documentation:
* __classcall_private__(cls, n, **kwds):
* __init__(self, n, use_monotone_triangles=False):
* __classcall_private__(cls, n, **kwds):
* __init__(self, n):
* __classcall_private__(cls, n, **kwds):
* __classcall_private__(cls, n, **kwds):
Missing doctests:
* _lattice_initializer(self):
* _lattice_initializer(self):
* _is_a_cover(mt0, mt1):
* _top_rows(row):
* _triangular_arrangements_base(row):
----------------------------------------------------------------------
}}}
- Since to_monotone_triangle and from_montone_triangle are inverses of
each other you could add some tests as follows:
{{{
sage: T = [[1, 2, 3], [1, 2], [1]]
sage: asm.to_monotone_triangle(asm.from_monotone_triangle(T)) == T
True
}}}
- Please add Examples to class AlternatingSignMatrices(Parent) below line
87. In fact I would suggest to add EXAMPLES to all of your classes so that
the user knows how to use them!
- Please use a line break on line 111 of your patch so that it is easier
to read.
- Line 180 of your patch it says "Return a couple to use in argument of
``Poset``. " Do you mean tuple instead of couple? Please add an EXAMPLES::
here so it is clear what the method does. A similar sentence appears in
line 363.
- As Frederic also mentioned, in line 282 you might want to use Lattice
instead of Poset.
- After line 324, please add an empty line.
- After line 374 the TESTS are missing!
- I would suggest to reuse all the doctests in the code that you are
deleting since you want your new code to give the same answers as before.
For example, I do not see the more systematic test such as
{{{
sage: [ AlternatingSignMatrices(n).cardinality() for n in range(0,
11)]
[1, 1, 2, 7, 42, 429, 7436, 218348, 10850216, 911835460,
129534272700]
}}}
But please reuse all of it in the appropriate places.
- I think if you are removing code or renaming methods, you first need to
"deprecate them" for one release cycle.
So much for now. Impressive work for a first patch!
Anne
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/12930#comment:8>
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.
