Closed monien closed 10 years ago
Description changed:
---
+++
@@ -6,5 +6,4 @@
- reduction to cusp
- identify cusp class
-There are a number of special cases where the straightforward implementation of the LLT algorithm fails.
-More testing is required.
+Added tests for LLT algorithm and reduction to cusp.
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.
Reviewer: David Loeffler
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.
Work Issues: LaTeX display doesn't work
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.
_latex_
in congroup.py
.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?
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.
latex as would produced by my new latex version
Attachment: test latex for Farey symbols.tiff.gz
use in sage worksheet
Attachment: test new Farey symbols -- Sage.pdf.gz
Attachment: farey.patch.gz
Farey symbol enhancement
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...
Branch: u/hmonien/ticket/15265
Added modified version of David's latex for FareySymbol.
Commit: 119c796
Branch pushed to git repo; I updated commit sha1. New commits:
119c796 | Version with modified latex for FareySymbol. |
Changed branch from u/hmonien/ticket/15265 to u/mraum/ticket/15265
Sorry for that, I'm not yet used to the new Trac system. I'm now changing the branch.
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 |
Changed reviewer 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.
Branch pushed to git repo; I updated commit sha1. New commits:
be4148a | trac 15265: Additional change to farey.cpp. |
Martin,
all the changes seem to be okay. I think there was one more test for odd and even groups which try to remember ... .
Branch pushed to git repo; I updated commit sha1. New commits:
f24bf98 | trac 15265: various changes, finishing review. |
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.
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.
Changed work issues from LaTeX display doesn't work to none
It's not wrong, and so let's keep it so that we don't break any architecture.
See #15719 for a documentation issue
The additional functionality included in this patch are:
Added tests for LLT algorithm and reduction to cusp.
CC: @vbraun @loefflerd
Component: modular forms
Keywords: Farey symbol, arithmetic subgroups
Author: Hartmut Monien
Branch/Commit: u/mraum/ticket/15265 @
f24bf98
Reviewer: David Loeffler, Martin Raum
Issue created by migration from https://trac.sagemath.org/ticket/15265