Opened 5 years ago

Closed 5 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 vripoll)

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 5 years ago by vripoll

  • Branch set to u/vripoll/ticket16120

comment:2 Changed 5 years ago by stumpc5

  • Commit set to d025f0fd7d20a0c51ebe1123919cd1940dd8d91b

What is wrong with using CLF rather than CDF? If the former is possible, I would prefer to use it.


New commits:

d025f0fAdded a __float__ method to the class Universal Cyclotomic Field

comment:3 Changed 5 years ago by vripoll

  • 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:

d025f0fAdded a __float__ method to the class Universal Cyclotomic Field

comment:4 follow-up: Changed 5 years ago by vripoll

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 5 years ago by vripoll

  • Authors set to Vivien Ripoll
  • Cc tmonteil added
  • Status changed from new to needs_review

comment:6 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by stumpc5

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 5 years ago by mmezzarobba

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 5 years ago by chapoton

  • 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:

c71e5d9trac #16120 correct doctest and python3 syntax
e5e01d0trac #16120 change to ValueError

comment:9 Changed 5 years ago by mmezzarobba

  • Status changed from needs_review to positive_review

comment:10 follow-ups: Changed 5 years ago by tmonteil

  • 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 5 years ago by mmezzarobba

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 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 ?

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) and CIF(e) work as well.

Sure. But not necessarily as part of this ticket.

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.

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 5 years ago by mmezzarobba

Replying to tmonteil:

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.

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 5 years ago by vripoll

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: Changed 5 years ago by chapoton

So, what remains to be done here ?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by mmezzarobba

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 5 years ago by tmonteil

Replying to 11:

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.

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) and CIF(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 to RR (hence to float) 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"?

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 5 years ago by tmonteil

  • 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 5 years ago by vbraun

  • Branch changed from public/ticket/16120 to e5e01d0f89b7b23e0f964e0ac286913de9efe1b4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.