#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 | 3c7ca0f2bf33d3fdec7b79c353b28ccc01cd419d
Branch: | Stopgaps:
u/jj/triangle_conjugacy |
Dependencies: #16936 |
-------------------------------------+-------------------------------------
Comment (by jj):
Hi Vincent
Thanks a lot for the feedback/review!
> 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.
Yeah, I realize that. I'm very sorry!
Most of the changes are related/interacting.
> - 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.
Mainly because it was originally a stub implementation.
But yes that could be changed. What about doing it in a separate ticket?
> - 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.
Matrix group operations are used at several places.
I choose MatrixGroupElement_generic because I considered it simpler
/ less error prone than reimplementing all operations.
If it makes a big difference for performance I can try to change it.
What about doing it in a separate ticket?
> - Is `string_rep` standard ? I thought that `str` would have been
better.
I used 'string_rep' as a helper function for _repr_
(since I have several possible representations) but 'str' would be
fine by me as well (if it isn't used for other purposes).
> - 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.
The way I documented my code I put the main documentation of classes
resp. their usage in the readme.py file.
> - 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`).
I tried to remove as many cached_methods as possible without causing a
major
performance drop. Some of them would cause an overall 30% drop in
performance
(according to doctests at least) if removed. One reason for this is also
that
group elements are not uniquely represented, so if the basic elements
aren't
cached several expensive calculations on them would be unnecessarily
repeated
(in particular simply displaying the element would probably take much
longer).
One idea might be to use UniqueRepresentation for elements as well but in
this
instance I am not really sure how (resp. what to put in __classcall__).
Again this is a change which is not related to the ticket so it could be
done in
a separate ticket.
> - 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`.
.is_real_positive seems to simply use .n() to numerically decide
positivity.
In particular it requires some embedding into e.g. RR or RIF.
I choose AA as the default embedding because it is the most versatile
base for further embeddings. If I "RR" or "RIF" would be used
then embeddings into AA wouldn't be easily possibly anymore.
I.e. I choose generality/flexibility/versatility over performance.
If performance becomes a big issue my suggestion would be to explicitely
specify an embedding for those cases. E.g. For fixed points (which live
in a relative number field extension) I needed this (in particular
because there is no default embedding) and there is the
root_extension_embedding method for that purpose.
> - What is the point of the method `m.root_extension_embedding()` ? You
may
> use directly `m.parent().XXX`.
For elements we know the discriminant which is given as an argument to
the parent function.
Jonas
--
Ticket URL: <http://trac.sagemath.org/ticket/16976#comment:20>
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.
