#16976: Conjugacy classes and rational period functions for Hecke triangle
groups
-------------------------------------+-------------------------------------
Reporter: jj | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.4
Component: modular forms | Resolution:
Keywords: hecke triangle | Merged in:
group conjugacy class rational | Reviewers:
period function | Work issues:
Authors: Jonas Jermann | Commit:
Report Upstream: N/A | 94dae505a9b10515afb7c0013412e4d92a01626d
Branch: | Stopgaps:
u/jj/triangle_conjugacy |
Dependencies: #16936 |
-------------------------------------+-------------------------------------
Changes (by vdelecroix):
* status: needs_review => needs_work
Comment:
Hello Jonas,
It is always a bad choice to make a lot of modifications in one patch
(especially because no reviewer will like it). From the description of
your ticket, it looks like you would better have 2 or 3.
Some remarks:
- Why did you choose to put the code for triangle groups in
`sage.modular.modform_hecketriangle`. As you can see elements of
arithmetic groups are in `sage.modular.arithgroup` while the code relative
to modular forms is in `sage.modular.modform`. People interested in Hecke
groups are not necessarily interested in modular forms.
- As you might have seen, the `ArithmeticSubgroupElement` class inherits
directly from `MultiplicativeGroupElement` and not from
`MatrixGroupElement_generic`. The reason is performance as most methods of
`2x2` matrices (such as det, traces, ...) are much faster using explicit
formulas. Moreover, the class for arithmetic group element only have four
slots for the elements a,b,c,d which is much simpler than a list of
vectors for storing rows.
- Is `string_rep` standard ? I thought that `str` would have been better.
- You must put '''useful''' tests in the class `HeckeTriangleGroupElement`
and move it from the `__init__` method to the main documentation of the
class. These examples are here to help the users. In particular, start
with code that works, not with potential failure.
- There are too much `cached_method`. In an ideal world there should be
none. A python objects takes some amount of RAM, and each cached method
stores python objects. Abusing with the `cached_method` implies that you
can not deal with huge list of Hecke triangle group element. You should
basically remove all of them unless you have a '''very''' strong argument
for having it (e.g. `root_extension_embedding` in `HeckeTriangleGroup`).
- Why do you use the algebraic field `AA` instead of number fields ? It is
much slower in all situations. Note that there is a (relatively slow)
method for checking that a number is positive: `.is_real_positive`.
- What is the point of the method `m.root_extension_embedding()` ? You may
use directly `m.parent().XXX`.
Vincent
--
Ticket URL: <http://trac.sagemath.org/ticket/16976#comment:17>
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.
