#8227: Improving iterated palindromic closure computation
-----------------------------+----------------------------------------------
Reporter: abmasse | Owner: abmasse
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.3.3
Component: combinatorics | Keywords: iterated palindromic closure
Author: abmasse | Upstream: N/A
Reviewer: | Merged:
Work_issues: |
-----------------------------+----------------------------------------------
Changes (by slabbe):
* status: needs_review => needs_work
Comment:
1. I think the patch should follow the python behavior (i.e. return -1)
{{{
sage: '0123456789'.find('4566')
-1
sage: '0123456789'.rfind('4566')
-1
}}}
for those functions :
{{{
sage: Word(range(10)).rfind(Word([4,5,6,6]))
sage: Word(range(10)).find(Word([4,5,6,6]))
}}}
2. The following comment found in the doc of `find` and `rfind`
{{{
This function is different for Word_str objects:
}}}
illustrates a problem : the behavior for `Word_str` should be the same
(`Word_str` is wrong). Can you fix it? You may consult #8127 for an idea
of how to handle it. Make sure to ask the parent using super if type of
other is not an str or a `Word_str`.
3. An enumeration in the doc of the iterator function is broken as seen in
the result of :
{{{
sage: w = Word(range(10))
sage:
browse_sage_doc(w._iterated_right_palindromic_closure_recursive_iterator)
}}}
Adding a blank line before the itemize should repair the problem. I also
suggest to put `WordMorphism`, `'recursive'` and
`_iterative_righ...iterator()` inside double backquotes (like input
arguments).
4. Looking the function below but also how naive is the code of `find`,
maybe the function `find` could use the function `first_pos_in` instead?
This makes me realize that the function `first_pos_in` was probably a bad
choice of name.... Using the new deprecation warning introduced recently
by Florent Hivert this (name modif) can be done more easily now (but not
in this ticket).
{{{
sage: %timeit Word([990,991,992,993]).first_pos_in(Word(range(1000)))
125 loops, best of 3: 1.65 ms per loop
sage: %timeit Word(range(1000)).find(Word([990,991,992,993]))
5 loops, best of 3: 48 ms per loop
}}}
5. Could `rfind` could be improved easily using `_pos_in` and other good
suffix table already implemented? If so, it can be good to do it now. But
if you don't care now, it is fine. The function could be improved later if
it is valuable. Anyway, the next step for all those search stuff is to be
cythoned...
That's all for my comments.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/8227#comment:4>
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.
