Closed seblabbe closed 7 years ago
Branch: u/moritz/21928
I added a _latex_
function to the ContinuedFraction_base
base class, that displays 10 terms by default. Notice that the subclass ContinuedFraction_periodic
has already its own _latex_
, printing all the terms.
New commits:
54eef88 | adding latex support for real continued fractions |
(I hope my automatic white space correction didn't screw up the ascii-art to much..)
Would be better to have only one place with _latex_
. With your version we still have
sage: K.<sqrt2> = QuadraticField(2)
sage: continued_fraction(sqrt2)
[1; (2)*]
sage: latex(continued_fraction(sqrt2))
Traceback (most recent call last):
...
NotImplementedError: latex not implemented for non rational continued fractions
Branch pushed to git repo; I updated commit sha1. New commits:
a645dac | fixing old doctest and adding a new one |
Thanks for proposing a fix to this issue. I just downloaded the branch. It works great.
Here are two relatively small comments that I would suggest to change before positive review:
I think the nterms=10
would be better put as an argument with a default value of 10 for the _latex_
method like it is done for the method str
of the same object. The first thing that will happen is that somebody want to see 13 terms (with the ...). I know that _latex_
method is used from latex
function which might not forward arguments *args, **kwds
. Something more complicated involving a new method could be done like here but I do not think it is necessary. To my opinion, I would just move nterms=10
to the argument which will allow somebody to see 13 terms (with the ...).
The two output lines in doctests are very long. I suggest to wrap them to keep less than 80 characters on each line. The doctest framework won't notice the newlines. Alternatively, the code could include well placed \n
so that \frac{\displaystyle
are all well aligned vertically.
Branch pushed to git repo; I updated commit sha1. New commits:
2765115 | allow nterms as argmument and more line breaks |
Branch pushed to git repo; I updated commit sha1. New commits:
d55256c | removed one line |
I addressed both points mentioned by slabbe; thanks for reviewing!
sage -grep continued_fraction
lists 20 files and sage -tp --long
on them gives all tests passed:
sage -tp --long src/sage/arith/misc.py src/sage/calculus/wester.py src/sage/combinat/words/word_generators.py src/sage/databases/oeis.py src/sage/geometry/cone.py src/sage/modular/modform_hecketriangle/hecke_triangle_group_element.py src/sage/modular/modform_hecketriangle/hecke_triangle_groups.py src/sage/modular/modform_hecketriangle/readme.py src/sage/modular/modsym/ambient.py src/sage/modular/modsym/modular_symbols.py src/sage/modular/pollack_stevens/manin_map.py src/sage/plot/misc.py src/sage/rings/all.py src/sage/rings/contfrac.py src/sage/rings/continued_fraction.py src/sage/rings/number_field/number_field_element_quadratic.pyx src/sage/rings/rational.pyx src/sage/rings/real_lazy.pyx src/sage/tests/book_stein_ent.py src/sage/tests/book_stein_modform.py
I am currently building the documentation to make sure...
Reviewer: Vincent Delecroix, Sébastien Labbé
Author: Moritz Firsching
Changed branch from u/moritz/21928 to d55256c
in the future, remember to use python3 syntax for print
CC: @videlec
Component: number theory
Keywords: days79
Author: Moritz Firsching
Branch:
d55256c
Reviewer: Vincent Delecroix, Sébastien Labbé
Issue created by migration from https://trac.sagemath.org/ticket/21928