Opened 9 years ago
Closed 8 years ago
#14971 closed enhancement (fixed)
Better latex for Farey symbols
Reported by:  davidloeffler  Owned by:  

Priority:  major  Milestone:  sage6.1 
Component:  modular forms  Keywords:  sd51 
Cc:  Merged in:  
Authors:  David Loeffler  Reviewers:  Martin Raum 
Report Upstream:  N/A  Work issues:  
Branch:  u/mraum/ticket/14971 (Commits, GitHub, GitLab)  Commit:  6d70fc410525d5e8ebe39db10f7ef91cbf663bf0 
Dependencies:  Stopgaps: 
Description
This patch adds a LaTeX rendering for Farey symbols (using the notation from Kurth and Long's paper, http://arxiv.org/abs/0710.1835).
Attachments (1)
Change History (12)
Changed 9 years ago by
comment:1 Changed 9 years ago by
Here's a patch. It also corrects the rendering of the fundamental domain in a certain corner case where we were previously plotting a fundamental domain that was too large.
comment:2 Changed 9 years ago by
 Status changed from new to needs_review
comment:3 Changed 9 years ago by
Dear David, I have been working on a major overhaul of the Farey symbols including among other major improvements the latex code for Farey symbols e.g.
sage: f=FareySymbol?(Gamma0(23)) sage: latex(f) \xymatrix{& \infty \ar@{}@/_{1pc}/[r]_{1}& 0 \ar@{}@/_{1pc}/[r]_{2}& \frac{1}{5} \ar@{}@/_{1pc}/[r]_{3}& \frac{1}{4} \ar@{}@/_{1pc}/[r]_{5}& \frac{1}{3} \ar@{}@/_{1pc}/[r]_{3}& \frac{1}{2} \ar@{}@/_{1pc}/[r]_{4}& \frac{2}{3} \ar@{}@/_{1pc}/[r]_{2}& \frac{3}{4} \ar@{}@/_{1pc}/[r]_{4}& \frac{4}{5} \ar@{}@/_{1pc}/[r]_{5}& 1 \ar@{}@/_{1pc}/[r]_{1}&\infty}
In my patch which I have to rework because the Farey symbol code in sage has been changed between 5.9 and 5.10 will include cusp_class and reduce_to_cusp. I have to remove some experimental features which are probably not useful for everybody.
So should I merge your changes into my patch?
comment:4 Changed 9 years ago by
Does that work in the Sage notebook? I don't know if the MathJax viewer for LaTeX formulae supports xypic.
Anyway, it looks like your patch will supersede this one here (I guess using xypic will give much nicer output than the rather simplistic approach based on \underbrace
which my patch uses). So if your code is ready before anyone gets round to reviewing this one, we can close this ticket. But let's keep it open for now.
comment:5 Changed 8 years ago by
 Branch set to u/mraum/ticket/14971
 Created changed from 07/25/13 19:18:48 to 07/25/13 19:18:48
 Modified changed from 09/12/13 09:01:04 to 09/12/13 09:01:04
comment:6 Changed 8 years ago by
 Commit set to 6d70fc410525d5e8ebe39db10f7ef91cbf663bf0
I turned this into a git branch. I kept David as the author, even though I took the freedom not to convert the second part of the patch, which is not concerned with latex output).
I can give a positive review to the parts that I converted, but I need somebody to sign off that we do not take over the second part. First, it doesn't respect the "show_tesselation" option. Second, it leads to code duplication for the sake of what I think is a very little and insignificant speed up.
New commits:
6d70fc4  trac 14971: Add LaTeX rendering for Farey symbols.

comment:7 Changed 8 years ago by
Hi Martin,
The second part of the patch, which you are proposing to reject, wasn't intended as a speedup: if I remember correctly (this was a while ago) the specialcase code was there to correct a wrong output that was being returned in a specific corner case, the unique index 2 subgroup of PSL2Z. Anyway it indeed has nothing to do with latex formatting, so it should probably have been a separate ticket all along.
David
comment:8 Changed 8 years ago by
Hi David:
That's good to hear. May I then propose that you give a positive review to leaving away the second part? I have reviewed and tested the rest of the ticket. (The patchbot doesn't seem to get the merging right. Any idea how to fix that, that is, let the patchbot know it is based on 6.0beta4?)
Please note that the index 2 subgroup is dealt with in Hartmut's updated FareySymbol?'s. I have made this ticket a dependency of Hartmut's to avoid having a Sage version which only has xymatrix output.
Martin
comment:9 Changed 8 years ago by
 Status changed from needs_review to positive_review
OK, that sounds like the right way to go. I'm afraid I don't know how to fix the patchbot!
comment:10 Changed 8 years ago by
 Reviewers set to Martin Raum
comment:11 Changed 8 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
Patch against 5.11.beta3