#15265 closed enhancement (fixed)
Additional functionality for Farey symbols
Reported by:  hmonien  Owned by:  hmonien 

Priority:  major  Milestone:  sage6.1 
Component:  modular forms  Keywords:  Farey symbol, arithmetic subgroups 
Cc:  vbraun, davidloeffler  Merged in:  
Authors:  Hartmut Monien  Reviewers:  David Loeffler, Martin Raum 
Report Upstream:  N/A  Work issues:  
Branch:  u/mraum/ticket/15265 (Commits)  Commit:  f24bf982ce00c4227cc20394c898dc0d078dcf96 
Dependencies:  Stopgaps: 
Description (last modified by )
The additional functionality included in this patch are:
 various possibilities for the tessellation of the fundamental domain (Dedekind, coset, none)
 membership test via the Lang, Lim, Tan algorithm
 improved LaTeX output (via xymatrix)
 reduction to cusp
 identify cusp class
Added tests for LLT algorithm and reduction to cusp.
Attachments (3)
Change History (28)
comment:1 Changed 6 years ago by
 Description modified (diff)
comment:2 Changed 6 years ago by
 Status changed from new to needs_review
comment:3 Changed 6 years ago by
comment:4 Changed 6 years ago by
 Reviewers set to David Loeffler
 Status changed from needs_review to needs_work
 Work issues set to LaTeX display doesn't work
OK, here's something a bit more important. The LaTeX display for Farey symbols doesn't work in the notebook: instead of rendering the Farey symbol it prints a little box with the raw LaTeX code in it. The problem is that the notebook uses MathJax for its latex rendering, and xypic is not supported by MathJax. I did warn you about this some while back.
Other than that the new code looks fine.
comment:5 Changed 6 years ago by
I do understand that the notebook (mathjax) currently does not support xymatrix. There are some attempts to make xymatrix also work for mathjax which would cure this problem. For the time being the situation is not very different from the one for using tikz with graphs. So the usual story:
latex.add_to_preamble('\usepackage[all]{xy}') latex.mathjax_avoid_list('xymatrix').
Then
view(FareySymbol(Gamma0(23)), engine='pdflatex', tightpage=True)
does produce a latex view of the Farey symbol. I would hope that xymatrix would be included in in mathjax sooner than later. Please have a look at http://sonoisa.github.io/xyjax/xyjax.html. One could of course check wether the latex is produced in a notebook or not but that might lead to inconsistencies for the user.
I'll fix the (minor) problems tomorrow. Thanks for the prompt review.
comment:6 Changed 6 years ago by
 Fixed typo in
_latex_
incongroup.py
.  Added proper commit message
comment:7 Changed 6 years ago by
I'm not sure that the comparison with using tikz for graphs is a fair one. There's clearly no hope of getting standard LaTeX to render graphs, so it's reasonable to require an extra package. On the other hand, one can produce a viable approximation to a Farey symbol without using any extra packages, as in my recent ticket #14971; this may not look quite so nice as your version, but it avoids these compatibility issues.
Can you add a doctest illustrating your changes to arithgroup_element.acton?
comment:8 Changed 6 years ago by
I have added two doctests to arithgroup_element.acton. I do understand your concern about the latex. The question here is: Is the graph for the Farey symbol just some equation or a graph. I would opt for the latter.
I have modified the _latex_
of the Farey symbol so that it does work with sagemath cloud.
To illustrate that it is fairly easy to use the current version of the latex output (one thing which
has been bugging me for a long time is the extra brackets in the latex output in a sage worksheet) I included two examples
how to use it.
The only other alternative I see is to add yet another property to the Farey symbol like graph() or the like to keep the nice latex format.
comment:9 Changed 6 years ago by
Just so that I understand correctly, there are two potential LaTeX outputs, one with (here) and one without xy (at #14971)? How about you do
if 'xymatrix' in latex.mathjax_avoid_list(): output_with_xy() else: output_without_xy()
Since xy is in the default latex preamble we should probably add it to the avoid list by default, but that would be for another ticket...
comment:10 Changed 6 years ago by
 Branch set to u/hmonien/ticket/15265
 Created changed from 10/09/13 09:31:44 to 10/09/13 09:31:44
 Modified changed from 11/22/13 15:50:24 to 11/22/13 15:50:24
comment:11 Changed 6 years ago by
Added modified version of David's latex for FareySymbol?.
comment:12 Changed 6 years ago by
 Commit set to 119c79638ecf2c368d66dc84da2b35c01c9470b9
Branch pushed to git repo; I updated commit sha1. New commits:
119c796  Version with modified latex for FareySymbol.

comment:13 Changed 6 years ago by
comment:14 Changed 6 years ago by
comment:15 Changed 6 years ago by
 Branch changed from u/hmonien/ticket/15265 to u/mraum/ticket/15265
 Commit changed from 119c79638ecf2c368d66dc84da2b35c01c9470b9 to 52108f0ba86531f90526c3b1978f3c43bb162956
Sorry for that, I'm not yet used to the new Trac system. I'm now changing the branch.
comment:16 Changed 6 years ago by
 Commit changed from 52108f0ba86531f90526c3b1978f3c43bb162956 to 7d78f7c0811682fbae3e9adb4b6e796241bd8b67
Branch pushed to git repo; I updated commit sha1. New commits:
248d2bc  trac 15265: Add some further examples and one test to _acton_ of elements of arithmetic groups.

961251a  trac 15265: Minor changes to farey.cpp and farey.hpp.

d82a0b6  trac 15265: Minor changes to sl2z.cpp and sl2z.hpp.

7d78f7c  trac 15265: Minor changes to farey_symbol.pyx

comment:17 Changed 6 years ago by
 Reviewers changed from David Loeffler to David Loeffler, Martin Raum
Hartmut, could you review the few changes that I made to your code? I have the following questions/comments, which you could help me with:
reference for rademacher_matrix in farey.cpp? farey.cpp:FareySymbol(istream& is) why not "is >> *this" ? farey.cpp:370 : it seems that x will always have length 1. farey.cpp:129 I understand this is tempting, but PRETTY_FUNCTION is gcc only. farey_symbol.pyx:plot rgbcolor option does not apply to Dedekind tesselation. Add one example each
I still need to test the code, but hopefully I'll have time for this tomorrow.
comment:18 Changed 6 years ago by
 Commit changed from 7d78f7c0811682fbae3e9adb4b6e796241bd8b67 to be4148aab60ab735bd0a361a735cba2d5e47b704
Branch pushed to git repo; I updated commit sha1. New commits:
be4148a  trac 15265: Additional change to farey.cpp.

comment:19 Changed 6 years ago by
Martin,
 the reference for the Rademacher matrix is http://www.jstor.org/stable/2371313 .
 what did you want to say about is >> *this?
 fair enough and true: len(x)==1 here
 PRETTY_FUNCTION is a leftover from testing
 true, what is a consistent naming scheme?
 I liked the format "sting" ;)
all the changes seem to be okay. I think there was one more test for odd and even groups which try to remember ... .
comment:20 Changed 6 years ago by
 Commit changed from be4148aab60ab735bd0a361a735cba2d5e47b704 to f24bf982ce00c4227cc20394c898dc0d078dcf96
Branch pushed to git repo; I updated commit sha1. New commits:
f24bf98  trac 15265: various changes, finishing review.

comment:21 Changed 6 years ago by
Ok, I made all necessary changes. Can you have a look?
Just one, last question: Why do you actually use extern "C"? farey_symbols.pyx is translated to a C++ file, so that shouldn't be necessary.
comment:22 Changed 6 years ago by
This was added by somebody  not me  in some previous version of SAGE to have it working on some particular machine. Otherwise everything looks fine now.
comment:23 Changed 6 years ago by
 Status changed from needs_work to positive_review
 Work issues LaTeX display doesn't work deleted
It's not wrong, and so let's keep it so that we don't break any architecture.
comment:24 Changed 6 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
comment:25 Changed 6 years ago by
See #15719 for a documentation issue
Pedantic comments: you need a proper commit message on the patch, or the release manager will complain; and there is a failing doctest in the
_latex_
method incongroup_gamma.py
(typo?). I'm running a more thorough doctest cycle now.