#14567: Refactor continued fractions
-------------------------------------+-------------------------------------
Reporter: vdelecroix | Owner: vdelecroix
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-6.2
Component: number theory | Resolution:
Keywords: continued | Merged in:
fractions, numerical | Reviewers: Ralf Stephan, Thierry
approximation | Monteil
Authors: Vincent Delecroix | Work issues:
Report Upstream: N/A | Commit:
Branch: | 8fbf106cbde07fed9c3f3ea2b32c9309185062af
u/vdelecroix/14567 | Stopgaps:
Dependencies: #13213, #13256, |
#14563 |
-------------------------------------+-------------------------------------
Changes (by vdelecroix):
* status: needs_work => needs_review
Comment:
Replying to [comment:33 tmonteil]:
> `CFF` is not fully autonomous and behaves like an overlay over other
Sage
> fields, thus sacrificing reliability over expressivity.
agreed. There is no transformation on input now. Most problem disappear.
The only trouble might be with floating point as I decided to first
convert the input into a rational. Note however that
{{{
sage: RR.random_element() in QQ
True
}}}
> I have not much problems with this overlay design as long as it is
clearly
> stated and consistent.
It is !
> Another issue is:
>
> {{{
> sage: CFF.is_exact()
> True
> }}}
>
> I disagree with that statement, since there are approximate elements in
CFF,
> for example `CFF(pi+0.1)`.
I do not want to make CFF inexact. The trouble comes from the unstability
of CFF... If you want a friendly CFF you either have to rebuild the
symbolic ring or do with it.
> in `_element_constructor_()`
>
> {{{
> - ``nterms`` -- integer (optional)
>
> ??? is that an old remaining thing ?
>
> - ``check`` -- boolean (optional) -- whether or not we check the
>
> "the" what ?
> }}}
This is now removed
> When you test whether an element of `SR` is a real number, you use
`RIF(x)`.
> Why not using `x.is_real()` (and make this method more reliable if
needed) ?
you can not believe in `is_real` but I used it.
> Another problem:
>
> {{{
> sage: a = RDF(0.1)
> sage: CFF(a)
> [0]
> }}}
>
> The reason is :
>
> {{{
> sage: (1/RIF(RDF(0.1))).unique_floor()
> ValueError: interval does not have a unique floor
> }}}
It is now correct.
{{{
sage: continued_fraction(0.1)
[0; 10]
}}}
> In the doc: "It is quite remarkable that".
>
> Add : "- any real number admits a unique continued fraction expansion"
done
> [...]
>
> I would propose : "from a list of numbers corresponding to its partial
> quotients."
All occurrences of digits are removed and replaced by partial quotients.
> Another problem:
>
> {{{
> sage: cf = CFF([(1,2,3),(1,2,4)]); cf.value()
> -1/86*sqrt229 + 139/86
>
> sage: sqrt229
> ....
> NameError: name 'sqrt229' is not defined
> }}}
I agree. It is quite hard to make it works. I only documented how to do
that in the method `value`.
> "two_last_convergents" -> "last_two_convergents"
done
> `_pn` and `_qn` looks good for internal variables, but i would suggest
to
> replace `pn()` and `qn()` methods by `numerator()` and `denominator()`
to stay
> consistent with rationals.
done
> In the `_mpfr_` method: "It is guaranteed that the result is exact"
>
> Floating point number are not exact, perhaps "accurate" is a better word
?
done
--
Ticket URL: <http://trac.sagemath.org/ticket/14567#comment:37>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
