#7145: Interval exchange transformations
-----------------------------+----------------------------------------------
Reporter: vdelecroix | Owner: vdelecroix
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.3.1
Component: combinatorics | Keywords:
Work_issues: | Author: Vincent Delecroix
Upstream: N/A | Reviewer:
Merged: |
-----------------------------+----------------------------------------------
Changes (by slabbe):
* status: needs_review => needs_work
Comment:
Dear Vincent,
Thanks for doing all those changes.
I am now looking at your recent patch and I have a few more comments. I am
sorry I was not able to tell them before. I will try now to make a
complete review with all my (hopefully) final remarks. After those FIVE
remarks answered, I think we will be very near of a positive review!
Sébastien
ONE.
> The resason is because it's possible to change the alphabet
> sage: p = PermutationIET('a b c','c b a')
> sage: p a b c c b a
> sage: p.alphabet = [1,2,3]
> sage: p 1 2 3 3 2 1
> sage: p.alphabet = 'cd'
> ...
> ValueError?: Your alphabet has not enough letters
I am not sure this is a good reason for using an attribute instead of a
method. See the following example :
{{{
sage: g = Graph()
sage: p = g.plot()
sage: p.xmin()
-1.0
sage: p.xmin(-2)
sage: p.xmin()
-2.0
}}}
TWO.
The documentation and doctest coverage is very good (96%) but not perfect
:
{{{
sla...@pol:~/sage-4.2/devel/sage-combinat/sage/combinat/iet$ sage
-coverage .
constructors.py: 75% (6 of 8)
iet.py: 100% (23 of 23)
labelled.py: 95% (57 of 60)
reduced.py: 100% (55 of 55)
strata.py: 100% (39 of 39)
template.py: 96% (89 of 92)
Overall weighted coverage score: 96.9%
Total number of functions: 277
We need 5 more function to get to 99% coverage.
}}}
Moreover, {{{sage -coverage *}}} tells many {{{function name doesn't occur
in doctests}}}. Maybe you could add {{{#indirect doctest}}} in doctests
that are indirect.
While you are at it, you may consider to add some INPUT and OUTPUT where
there are missing. See the discussion [http://groups.google.com/group
/sage-
devel/browse_thread/thread/7d529c646c685877/3f7678e55f60759f?hl=en&lnk=gst&q=coverage#3f7678e55f60759f
sage-devel: input and output in docstrings]
THREE.
I am getting 2 doctests failures :
{{{
sage -t "devel/sage-combinat/sage/combinat/iet/constructors.py"
**********************************************************************
File "/home/slabbe/sage-4.2/devel/sage-
combinat/sage/combinat/iet/constructors.py", line 531:
sage: for p in P: print p, "* *\n"
Expected:
a b
b a
* *
Got:
a b
b a * *
<BLANKLINE>
b a
a b * *
<BLANKLINE>
**********************************************************************
File "/home/slabbe/sage-4.2/devel/sage-
combinat/sage/combinat/iet/constructors.py", line 536:
sage: for p in P: print p, "\* * *\n"
Expected:
a b c
c b a
* * *
Got:
a b c
b c a \* * *
<BLANKLINE>
a b c
c a b \* * *
<BLANKLINE>
a b c
c b a \* * *
<BLANKLINE>
a c b
b a c \* * *
<BLANKLINE>
a c b
b c a \* * *
<BLANKLINE>
a c b
c b a \* * *
<BLANKLINE>
b a c
a c b \* * *
<BLANKLINE>
b a c
c a b \* * *
<BLANKLINE>
b a c
c b a \* * *
<BLANKLINE>
b c a
a b c \* * *
<BLANKLINE>
b c a
a c b \* * *
<BLANKLINE>
b c a
c a b \* * *
<BLANKLINE>
c a b
a b c \* * *
<BLANKLINE>
c a b
b a c \* * *
<BLANKLINE>
c a b
b c a \* * *
<BLANKLINE>
c b a
a b c \* * *
<BLANKLINE>
c b a
a c b \* * *
<BLANKLINE>
c b a
b a c \* * *
<BLANKLINE>
**********************************************************************
1 items had failures:
2 of 6 in __main__.example_4
***Test Failed*** 2 failures.
For whitespace errors, see the file
/home/slabbe/.sage//tmp/.doctest_constructors.py
[3.0 s]
exit code: 1024
}}}
FOUR.
I use sage-4.2 and I have problem to docbuild a branch of sage. It
docbuilds the sage main branch, so I am not able to test if there are no
Sphinx warnings and if everythings looks good on the html doc. I am
waiting sage-4.3 to come out to test your patch on it and see the doc.
FIVE.
I changed a little bit the {{{_repr_}}} of Interval exchange
transformation. See the patch attached. Hope you agree.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/7145#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.
