Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#15265 closed enhancement (fixed)

Additional functionality for Farey symbols

Reported by: hmonien Owned by: hmonien
Priority: major Milestone: sage-6.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 hmonien)

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)

test latex for Farey symbols.tiff (272.0 KB) - added by hmonien 6 years ago.
latex as would produced by my new latex version
test new Farey symbols -- Sage.pdf (79.5 KB) - added by hmonien 6 years ago.
use in sage worksheet
farey.patch (49.5 KB) - added by hmonien 6 years ago.
Farey symbol enhancement

Download all attachments as: .zip

Change History (28)

comment:1 Changed 6 years ago by hmonien

  • Description modified (diff)

comment:2 Changed 6 years ago by hmonien

  • Status changed from new to needs_review

comment:3 Changed 6 years ago by davidloeffler

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 in congroup_gamma.py (typo?). I'm running a more thorough doctest cycle now.

comment:4 Changed 6 years ago by davidloeffler

  • 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 hmonien

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 hmonien

  • Fixed typo in _latex_ in congroup.py.
  • Added proper commit message

comment:7 Changed 6 years ago by davidloeffler

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 hmonien

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.

Last edited 6 years ago by hmonien (previous) (diff)

Changed 6 years ago by hmonien

latex as would produced by my new latex version

Changed 6 years ago by hmonien

use in sage worksheet

Changed 6 years ago by hmonien

Farey symbol enhancement

comment:9 Changed 6 years ago by vbraun

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 hmonien

  • 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 hmonien

Added modified version of David's latex for FareySymbol?.

comment:12 Changed 6 years ago by git

  • Commit set to 119c79638ecf2c368d66dc84da2b35c01c9470b9

Branch pushed to git repo; I updated commit sha1. New commits:

119c796Version with modified latex for FareySymbol.

comment:13 Changed 6 years ago by mraum

We did some parallel work. Hope it's ok for you to base the review on my branch (to which I already made some small changes).


New commits:

9da738atrac 15265: improvements for FareySymbol.
119c796Version with modified latex for FareySymbol.

comment:14 Changed 6 years ago by mraum

Another try to change the attached commit.


New commits:

9da738atrac 15265: improvements for FareySymbol.
119c796Version with modified latex for FareySymbol.

comment:15 Changed 6 years ago by mraum

  • 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 git

  • Commit changed from 52108f0ba86531f90526c3b1978f3c43bb162956 to 7d78f7c0811682fbae3e9adb4b6e796241bd8b67

Branch pushed to git repo; I updated commit sha1. New commits:

248d2bctrac 15265: Add some further examples and one test to _acton_ of elements of arithmetic groups.
961251atrac 15265: Minor changes to farey.cpp and farey.hpp.
d82a0b6trac 15265: Minor changes to sl2z.cpp and sl2z.hpp.
7d78f7ctrac 15265: Minor changes to farey_symbol.pyx

comment:17 Changed 6 years ago by mraum

  • 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 git

  • Commit changed from 7d78f7c0811682fbae3e9adb4b6e796241bd8b67 to be4148aab60ab735bd0a361a735cba2d5e47b704

Branch pushed to git repo; I updated commit sha1. New commits:

be4148atrac 15265: Additional change to farey.cpp.

comment:19 Changed 6 years ago by hmonien

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 git

  • Commit changed from be4148aab60ab735bd0a361a735cba2d5e47b704 to f24bf982ce00c4227cc20394c898dc0d078dcf96

Branch pushed to git repo; I updated commit sha1. New commits:

f24bf98trac 15265: various changes, finishing review.

comment:21 Changed 6 years ago by mraum

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 hmonien

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 mraum

  • 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 vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:25 Changed 6 years ago by vbraun

See #15719 for a documentation issue

Note: See TracTickets for help on using tickets.