Opened 7 years ago
Closed 7 years ago
#16120 closed enhancement (fixed)
Add a __float__ method to the class Universal Cyclotomic Field
Reported by:  vripoll  Owned by:  

Priority:  minor  Milestone:  sage6.2 
Component:  number fields  Keywords:  Cyclotomic field, float, days57 
Cc:  jipilab, stumpc5, sagecombinat, tmonteil  Merged in:  
Authors:  Vivien Ripoll  Reviewers:  Frédéric Chapoton, Marc Mezzarobba, Thierry Monteil 
Report Upstream:  N/A  Work issues:  
Branch:  e5e01d0 (Commits)  Commit:  e5e01d0f89b7b23e0f964e0ac286913de9efe1b4 
Dependencies:  Stopgaps: 
Description (last modified by )
There is currently no float method for the class Universal Cyclotomic Field. For example:
sage: UCF.<E> = UniversalCyclotomicField() sage: float(E(5)+E(5)^(1))  TypeError Traceback (most recent call last) <ipythoninput20d63f957cb28> in <module>() > 1 float(E(Integer(5))+E(Integer(5))**(Integer(1))) TypeError: float() argument must be a string or a number sage: float(CDF(E(5)+E(5)^(1)).real_part()) 0.6180339887498949
We would like something such as:
sage: float(E(5)+E(5)^(1)) 0.6180339887498949
This ticket is a prerequisite to #15703.
Change History (18)
comment:1 Changed 7 years ago by
 Branch set to u/vripoll/ticket16120
comment:2 Changed 7 years ago by
 Commit set to d025f0fd7d20a0c51ebe1123919cd1940dd8d91b
comment:3 Changed 7 years ago by
 Description modified (diff)
 Summary changed from Add a _float_ method to the class Universal Cyclotomic Field to Add a __float__ method to the class Universal Cyclotomic Field
New commits:
d025f0f  Added a __float__ method to the class Universal Cyclotomic Field

comment:4 followup: ↓ 6 Changed 7 years ago by
I don't know, but what is better with CLF? A timing test is favorable towards CDF:
sage: UCF.<E> = UniversalCyclotomicField() sage: b=E(5)+E(5)^(1) sage: %timeit float(CDF(b).real_part()) 1000 loops, best of 3: 1.06 ms per loop sage: %timeit float(CLF(b)) 100 loops, best of 3: 3.41 ms per loop
comment:5 Changed 7 years ago by
 Cc tmonteil added
 Status changed from new to needs_review
comment:6 in reply to: ↑ 4 ; followup: ↓ 7 Changed 7 years ago by
Replying to vripoll:
I don't know, but what is better with CLF?
The ComplexLazyField gives you as much precision as you want, while the ComplexDoubleField is restricted
sage: a = CLF(e) sage: b = CDF(e) sage: a 2.718281828459046? sage: b 2.71828182846
comment:7 in reply to: ↑ 6 Changed 7 years ago by
Replying to stumpc5:
The ComplexLazyField gives you as much precision as you want, while the ComplexDoubleField is restricted
But __float__
is supposed to return a Python float, so you don't need the extra precision...
Vivien: wouldn't a ValueError
be more appropriate in the case where the element you want to convert is not real?
comment:8 Changed 7 years ago by
 Branch changed from u/vripoll/ticket16120 to public/ticket/16120
 Commit changed from d025f0fd7d20a0c51ebe1123919cd1940dd8d91b to e5e01d0f89b7b23e0f964e0ac286913de9efe1b4
 Reviewers set to Frédéric Chapoton
I have corrected the doctest, that was failing. And also changed to ValueError?
And also used python3 syntax
This looks good to me. If nobody else disagrees, and my changes are ok, you can set to positive review.
New commits:
c71e5d9  trac #16120 correct doctest and python3 syntax

e5e01d0  trac #16120 change to ValueError

comment:9 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:10 followups: ↓ 11 ↓ 12 Changed 7 years ago by
 Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Thierry Monteil
 Status changed from positive_review to needs_info
I was working on this review but you are all too fast for me :)
I have some issues with testing this method.
sage: UCF.<E> = UniversalCyclotomicField() sage: e = 3/2*E(7) + E(7)^3 + E(7)^4 + 3/2*E(7)^6 sage: e.is_real() True sage: CDF(e) 0.0685316697714 + 1.62630325873e19*I
Meaning that somewhere the rounding is not correct (at least for the imaginary
part) with the computation of CDF(e)
. Hence float(CDF(self).real_part())
looks like a postprocessing of an error somewhere. But i do not see any
reason why the rounding of the real part will be better.
As your number is already real, i would prefer to do float(AA(e))
: here the
"real part projection" is done on the exact side (and the method real_exact
of AA
will be called only once). It is however between two and three times
slower.
What do you think about this ?
By the way, it could be nice to be able to have RR(e)
, RIF(e)
and CIF(e)
work as well.
That said:
sage: UCF.<E> = UniversalCyclotomicField() sage: e = 3/2*E(7) + E(7)^3 + E(7)^4 + 3/2*E(7)^6 sage: float(e) == float(AA(e)) False sage: float(e) < float(AA(e)) True sage: RIF = RealIntervalField(100) sage: RIF(float(e)).endpoints()[0] < RIF(AA(e)).endpoints()[0] < RIF(AA(e)).endpoints()[1] < RIF(float(AA(e))).endpoints()[0] True sage: RIF(AA(e)).endpoints()[1]  RIF(float(e)).endpoints()[0] 5.9704982077798935821915405952e19 sage: RIF(float(AA(e))).endpoints()[0]  RIF(AA(e)).endpoints()[1] 1.3280737987036467397076241791e17
Meaning that e
is closer to float(e)
than float(AA(e))
which are both
floating point numbers of the same precision, and the default rounding mode is
done "to the nearest". The conversion from AA
to RR
(hence to float
) is
assuming to respect the rounding mode, since the doc explicitely claims to
respect the rounding mode. This has to be explored, though it is not part of
this ticket.
comment:11 in reply to: ↑ 10 Changed 7 years ago by
Replying to tmonteil:
As your number is already real, i would prefer to do
float(AA(e))
: here the "real part projection" is done on the exact side (and the methodreal_exact
ofAA
will be called only once). It is however between two and three times slower.What do you think about this ?
I prefer the current version, because, as far as I understand, the test that e
is real implied by AA(e)
(AA._element_constructor_
testing that the imaginary part of QQbar(e)
is zero) can be much more expensive than e.is_real()
. And I don't really see the point of computing an exact representation of the real part if what we want in the end is an approximation.eans that
By the way, it could be nice to be able to have
RR(e)
,RIF(e)
andCIF(e)
work as well.
Sure. But not necessarily as part of this ticket.
The conversion from
AA
toRR
(hence tofloat
) is assuming to respect the rounding mode, since the doc explicitely claims to respect the rounding mode.
From AA
to RR
, yes, but why "hence to float
"?
That being said, I agree that the conversion from AA
to RR
does not seem to round to nearest:
sage: a = AA(e); r = RR(a) sage: r.exact_rational()  a 1.328?e17 sage: r.nextbelow().exact_rational()  a 6.0?e19
comment:12 in reply to: ↑ 10 Changed 7 years ago by
Replying to tmonteil:
The conversion from
AA
toRR
(hence tofloat
) is assuming to respect the rounding mode, since the doc explicitely claims to respect the rounding mode.
Hmm, are you referring to the docstring of AlgebraicReal.real_number()
? It states that "The approximation will be off by at most two ulp's" and "the rounding mode of the field is respected"... but what can it possibly mean to respect RNDN without guaranteeing an error < 1/2 ulp???
comment:13 Changed 7 years ago by
Thank you Fred for your review and correction of syntax. Thanks Marc and Thierry for the technical discussion, I'm learning a lot! I'll wait for some consensus before doing any change.
comment:14 followup: ↓ 15 Changed 7 years ago by
So, what remains to be done here ?
comment:15 in reply to: ↑ 14 ; followup: ↓ 16 Changed 7 years ago by
Replying to chapoton:
So, what remains to be done here ?
I don't know: I am happy with the patch as it is, but Thierry seemed to disagree, and I'd like to hear his opinion on my last few comments...
comment:16 in reply to: ↑ 15 Changed 7 years ago by
Replying to 11:
I prefer the current version, because, as far as I understand, the test that
e
is real implied byAA(e)
(AA._element_constructor_
testing that the imaginary part ofQQbar(e)
is zero) can be much more expensive thane.is_real()
. And I don't really see the point of computing an exact representation of the real part if what we want in the end is an approximation.
You are right about the timings (the AA
method is between 2 and 3 times slower). But the advantage with float(AA(e))
is that you have a direct conversion from AA
to RR
, not a conversion inherited from the coercion mechanism, which is less easy to control (unless all parts of Sage are floatingpoint accurate, which is unfortunately not true).
By the way, it could be nice to be able to have
RR(e)
,RIF(e)
andCIF(e)
work as well.Sure. But not necessarily as part of this ticket.
Of course, this was only a suggestion for a further work on universal cyclotomic field.
The conversion from
AA
toRR
(hence tofloat
) is assuming to respect the rounding mode, since the doc explicitely claims to respect the rounding mode.From
AA
toRR
, yes, but why "hence tofloat
"?
Because the method __float__
of AA
is just return float(RR(self))
.
Replying to 12:
Hmm, are you referring to the docstring of
AlgebraicReal.real_number()
? It states that "The approximation will be off by at most two ulp's" and "the rounding mode of the field is respected"... but what can it possibly mean to respect RNDN without guaranteeing an error < 1/2 ulp???
Yes, this is meaningless and i plan to fix it soon, see #16163. It should not cost much more time than now to have the right rounding since the current method already uses MPFI, it is only a matter of used digits (the current choice could have made sense if it was using CPU 53 bits precision arithmetics).
Replying to 15:
I don't know: I am happy with the patch as it is, but Thierry seemed to disagree, and I'd like to hear his opinion on my last few comments...
If you are all ok with the current implementation, i am fine with it, especially i understood that this method is created for plotting. I will make stronger tests, if necessary i will open a new ticket, but i do not want to block this.
comment:17 Changed 7 years ago by
 Reviewers changed from Frédéric Chapoton, Thierry Monteil to Frédéric Chapoton, Marc Mezzarobba, Thierry Monteil
 Status changed from needs_info to positive_review
comment:18 Changed 7 years ago by
 Branch changed from public/ticket/16120 to e5e01d0f89b7b23e0f964e0ac286913de9efe1b4
 Resolution set to fixed
 Status changed from positive_review to closed
What is wrong with using CLF rather than CDF? If the former is possible, I would prefer to use it.
New commits:
Added a __float__ method to the class Universal Cyclotomic Field