#11709: FareySymbol
-----------------------------+----------------------------------------------
Reporter: hmonien | Owner: craigcitro
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-4.7.2
Component: modular forms | Keywords: Farey symbol
Work_issues: | Upstream: N/A
Reviewer: Martin Raum | Author: Hartmut Monien
Merged: | Dependencies: None
-----------------------------+----------------------------------------------
Changes (by mraum):
* status: new => needs_review
* reviewer: => Martin Raum
Comment:
That is great work! Not so many issues that I found; And really this is
code of awesome readability.
The first major issue is the missing GPL header farey.cpp.
I will do the rest per line and file:
farey.cpp:
l.13 missing #include <algorithm> Did this really compile for you? That
seems impossible.
l.114 reverse the order of name and group (for consistency only).
l.127 this is bad. Compare with l.114. The easiest will be to change the
latter.
l.147 change to = a[i]/b[i] - a[i-1]/b[i-1], since operator/ already
constructs a new instance and hence it would be a wast to call the copy
constructor.
l.150 this is asking for trouble on Sun and other platforms. It would hurt
to use int i, will it?
l.154 add break; hereafter to make it a tiny bit faster.
l.187 and l.188 use A = ... and B = ..., since again the copy constructor
is not needed.
l.203 like l.187f
l.225 why don't you use throw like in l.260?
l.264 move this directly behind the alternate version of paring_matrix,
that is, l.247, to improve readability.
l.290 and l.291 define one_half(1,2) and use this here.
l.297 use one_half(1,2), which is a bit faster.
l.314 again this is calling for trouble on Sun. Use int k?
l.324 missing initialization c(paring.size(), 0);.
l.426 delete this assertion?
l.428 it is a shame that the compiler doesn't catch this, but again Sun
won't like this.
l.512 I'm not sure at all, but should this be NO, because already 1
represents a paired fraction? In that case you can remove the FREE label
from the enum.
farey_symbols.pyx:
l.5 the syntax is:
{{{
AUTHORS:
- Hartmut Monien (08 - 2011)
}}}
l.7 read C++.
l.46 Why to use the underscore at the end of the line?
l.50 Read `G`, because it is a mathematical symbol.
l.61, l.191, l.214, and l.267 use Sequence(..., cr = True) to make the
output more readable.
l.103 use _repr_.
l.109 sage: repr(FareySymbol(Gamma0(23)))
l.128, l.139, l.151, l.187, l.224, and l.281 missing full stop.
farey_symbol.h: You don't need this, but I think it would be best to use
it to replace l.21ff in farey.cpp.
sl2z.hpp:
l.25 and l.26 protected? otherwise it makes no sense to declare all the
functions above as friend.
Tests:
The tests are currently not sufficient to cover all cases the code treats.
This the difference occures in farey.cpp,l.189ff, it would be sufficient
to add tests like FareySymbol(G). Please add tests for SL2Z, Gamma1(4) and
GammaH(11, [2,7]) or any equivalent set of test cases.
I need to double check this agains Magma, and I will do this tomorrow. It
would also be good to see, whether everything works fine on the Sun test
cluster. I hope the patch bot will pick this up soon.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11709#comment:3>
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.
