#16976: Conjugacy classes and rational period functions for Hecke triangle
groups
-------------------------------------+-------------------------------------
Reporter: jj | Owner:
Type: enhancement | Status: needs_info
Priority: major | Milestone: sage-6.5
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 response. I am sorry that you are so unhappy with the
code. :(
Regarding the documentation:
This could be done in a separate ticket (it relates to much more than just
this ticket which is already rather large). I followed the advice of
Martin Raum when I started, sorry if the documentation is in the wrong
spot.
Regarding the cached methods:
Of course this is not about __repr__ (it only comes into play if the
representation method is changed). The result of the cached calculation is
rather short compared to the possibly really
long computations to get it (for all examples above). More importantly the
cached results are usually reused a lot. So in my opinion the overhead of
having a cached method is justified.
The information in basic_data, primitive_block_data and
continued_fractions are (in my opinion) not approximately equivalent. We
also don't always cache all methods. Instead different cached method have
different use cases (but yes there is some hiearchy between them):
(1) If the full block data is needed (not only _primitive_block_data) then
the _block_data caching helps. This is a less used case I guess but still
the caching helps a lot for that case. Indeed this will also use
_primitive_block_data.
(2) If only _primitive_block_data is needed, we don't need to have the
_block_data, _primitive_block_data is enough. For instance "hyperbolic
binary quadratic forms" are in 1-1 relation to primitive hyperbolic
matrices. So I guess this is the most important case.
Also: Indeed _primitive_block_data relies on continued_fraction.
(3) Block decomposition has a rather specific application (basically
determining the conjugacy type). We also want to be able to cache
continued fraction calculations (in particular since they're probably most
costly of the cached methods) without the need to also add unrelated block
decomposition calculations and data. So I left the caching for
continued_fraction in place. Note that continued fractions are sometimes
enough and no primitive block data is needed.
(4) _basic_data is related to the decomposition of the matrix as a product
of its generators S, T.
it is yet another non-equivalent use case. The whole base code is built on
the basic decomposition.
For instance it is used to determine if a given matrix even lies in the
group. It is used heavily for calculations with modular forms / upper half
plane action (to determine the automorphy factor, for calculations
involving the fundamental domain or representatives in it). So I decided
to always call _basic_data (unless check is disabled in which case nothing
is cached by default).
Regarding UniqueRepresentation:
I was just asking about it as an alternative to method caching (I myself
also decided against it).
I hope the summary above helped. I understand that you are unhappy with
the current implementation.
I am not happy about the idea of further removing cached methods (I did
some test a while ago: in each case removing a cached method had
significant performance impact). But if it is an absolute requirement I
would remove the caching for _block_data and _primitive_block_data. As
mentioned this will however have a really large negative performance
effect on methods that implicitely use them. :-(
Is the overhead of an unused cache method so large?
If yes, shouldn't the caching mechanism be improved instead?
Regarding the TODO on line 2700:
I added it because I am not familiar enough with all facets of the topic
(in particular geometry)
otherwise I would have improved it. But I know it is of interest to some
people.
So I guess (unfortunately) nobody will improve it.
Maybe the TODO remark should be removed all together since the current
documentation seems acceptable.
If the ticket modifications are better "in" than "out" then my suggestion
and hope would be to commit something "close" to the current form and to
apply more general modifications beyond the scope of just this ticket at a
later time (in a different ticket).
Regards
Jonas
--
Ticket URL: <http://trac.sagemath.org/ticket/16976#comment:25>
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.
