#16935: Faster palindromes function for the Words library
-------------------------------------+-------------------------------------
Reporter: nadialafreniere | Owner:
Type: enhancement | Status: new
Priority: major | Milestone: sage-6.4
Component: combinatorics | Resolution:
Keywords: words, finite | Merged in:
words, palindromes | Reviewers:
Authors: Nadia Lafrenière | Work issues:
Report Upstream: N/A | Commit:
Branch: | ca9a46b527309d80a785b8d0090f66b7691e1c9d
u/nadialafreniere/16935 | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Comment (by slabbe):
I did a first quick reading of the code. Here are some comments about it.
I still need to actually read the code itself, which I will do in another
comment not today.
1. There are two failing doctests in {{{finite_word.py}}}. Also, some
tabulation were introduced in that file. According to the
[http://sagemath.org/doc/developer/coding_basics.html#python-code-style
Python code style], the convention is to use 4 spaces for indentation
levels.
{{{
sage -t src/sage/combinat/words/finite_word.py
**********************************************************************
File "src/sage/combinat/words/finite_word.py", line 2736, in
sage.combinat.words.finite_word.FiniteWord_class.defect
Failed example:
Word('a').defect(f)
Exception raised:
Traceback (most recent call last):
...
IndexError: list index out of range
**********************************************************************
File "src/sage/combinat/words/finite_word.py", line 2740, in
sage.combinat.words.finite_word.FiniteWord_class.defect
Failed example:
Word('aa').defect(f)
Exception raised:
Traceback (most recent call last):
...
IndexError: list index out of range
**********************************************************************
1 item had failures:
2 of 26 in sage.combinat.words.finite_word.FiniteWord_class.defect
Error: TAB character found at lines 2463,2488,2509,2554
[1116 tests, 2 failures, 13.93 s]
----------------------------------------------------------------------
sage -t src/sage/combinat/words/finite_word.py # Tab character found
sage -t src/sage/combinat/words/finite_word.py # 2 doctests failed
----------------------------------------------------------------------
}}}
2. The name of the method `long_lpc` is not communicative. I would suggest
to use `length_longest_palindrome` instead.
3. There are missing commas in input docstrings:
{{{
#!diff
- - ``j`` -- integer a position that is the symmetry axis of the
palindrome.
+ - ``j`` -- integer, position that is the symmetry axis of the
palindrome.
- - ``m`` -- integer (default: 0) the minimal length of the
palindrome, if known.
+ - ``m`` -- integer (default: 0), minimal length of the
palindrome, if known.
}}}
4. Missing spaces in the argument of a function:
{{{
#!diff
- sage: Word('01010').long_lpc(j=3,f='0->1,1->0')
+ sage: Word('01010').long_lpc(j=3, f='0->1,1->0')
}}}
5. [http://legacy.python.org/dev/peps/pep-0008/#whitespace-in-expressions-
and-statements White spaces] are wrong here:
{{{
#!python
#Initialize m if set to 0
if m==0:
m=-(j%2)
}}}
and many other places.
6. Why? No space was OK.
{{{
#!diff
- INPUT:
+ INPUT :
}}}
7. Why do you erase and modify doctests in the `palindromes` method?
8. There is *a lot* of comments in the code. Are all of them really
necessary? I have never seen Sage code with that many comments. If the
algorithm is really complicated, the code can be simplified by moving
parts into another method. Also, the pseudo code of it can be put in the
doctrings. But, I would leave the code with less comments. See also :
http://legacy.python.org/dev/peps/pep-0008/#comments
--
Ticket URL: <http://trac.sagemath.org/ticket/16935#comment:5>
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 unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
