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: | sage-6.2 |
Component: | number fields | Keywords: | Cyclotomic field, float, days57 |
Cc: | jipilab, stumpc5, sage-combinat, 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) <ipython-input-2-0d63f957cb28> 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 follow-up: ↓ 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 ; follow-up: ↓ 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 follow-ups: ↓ 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.62630325873e-19*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 post-processing 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.9704982077798935821915405952e-19 sage: RIF(float(AA(e))).endpoints()[0] - RIF(AA(e)).endpoints()[1] 1.3280737987036467397076241791e-17
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?e-17 sage: r.nextbelow().exact_rational() - a -6.0?e-19
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 follow-up: ↓ 15 Changed 7 years ago by
So, what remains to be done here ?
comment:15 in reply to: ↑ 14 ; follow-up: ↓ 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 floating-point 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