#21928 closed defect (fixed)
produce nice latex code for latex(continued_fraction(pi)) and other continued fractions
Reported by: | slabbe | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.5 |
Component: | number theory | Keywords: | days79 |
Cc: | vdelecroix | Merged in: | |
Authors: | Moritz Firsching | Reviewers: | Vincent Delecroix, Sébastien Labbé |
Report Upstream: | N/A | Work issues: | |
Branch: | d55256c (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
sage: latex(continued_fraction(pi)) \text{\texttt{[3;{ }7,{ }15,{ }1,{ }292,{ }1,{ }1,{ }1,{ }2,{ }1,{ }3,{ }1,{ }14,{ }2,{ }1,{ }1,{ }2,{ }2,{ }2,{ }2,{ }...]}}
Change History (17)
comment:1 Changed 6 years ago by
- Branch set to u/moritz/21928
comment:2 Changed 6 years ago by
- Commit set to 54eef889d5d6e33368b64b77ab34cc730a02968a
comment:3 Changed 6 years ago by
(I hope my automatic white space correction didn't screw up the ascii-art to much..)
comment:4 Changed 6 years ago by
- Status changed from new to needs_review
comment:5 Changed 6 years ago by
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
comment:6 Changed 6 years ago by
- Commit changed from 54eef889d5d6e33368b64b77ab34cc730a02968a to 335bf30fddda1e8f452171b5233bc56a2c93f261
comment:7 Changed 6 years ago by
- Commit changed from 335bf30fddda1e8f452171b5233bc56a2c93f261 to a645dacbb7f3f20f6425fc310198c9c111450b9b
Branch pushed to git repo; I updated commit sha1. New commits:
a645dac | fixing old doctest and adding a new one
|
comment:8 Changed 6 years ago by
- Summary changed from latex(continued_fraction(pi)) does not return nice latex code to produce nice latex code for latex(continued_fraction(pi)) and other continued fractions
comment:9 Changed 6 years ago by
- Status changed from needs_review to needs_work
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 methodstr
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 fromlatex
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 moventerms=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.
comment:10 Changed 6 years ago by
- Commit changed from a645dacbb7f3f20f6425fc310198c9c111450b9b to 2765115750e7f236e1c923a66d8172ed4e71adbe
Branch pushed to git repo; I updated commit sha1. New commits:
2765115 | allow nterms as argmument and more line breaks
|
comment:11 Changed 6 years ago by
- Commit changed from 2765115750e7f236e1c923a66d8172ed4e71adbe to d55256c8879ae3a80b685e74540e738360ed857d
Branch pushed to git repo; I updated commit sha1. New commits:
d55256c | removed one line
|
comment:12 Changed 6 years ago by
- Status changed from needs_work to needs_review
I addressed both points mentioned by slabbe; thanks for reviewing!
comment:13 Changed 6 years ago by
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...
comment:14 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:15 Changed 6 years ago by
- Reviewers set to Vincent Delecroix, Sébastien Labbé
comment:16 Changed 6 years ago by
- Branch changed from u/moritz/21928 to d55256c8879ae3a80b685e74540e738360ed857d
- Resolution set to fixed
- Status changed from positive_review to closed
comment:17 Changed 5 years ago by
- Commit d55256c8879ae3a80b685e74540e738360ed857d deleted
in the future, remember to use python3 syntax for print
I added a
_latex_
function to theContinuedFraction_base
base class, that displays 10 terms by default. Notice that the subclassContinuedFraction_periodic
has already its own_latex_
, printing all the terms.New commits:
adding latex support for real continued fractions