Opened 3 years ago
Last modified 5 weeks ago
#26927 needs_work defect
nf > float conversion should be fast
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage9.6 
Component:  number theory  Keywords:  
Cc:  slelievre  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Daniel Krenn 
Report Upstream:  N/A  Work issues:  failing doctests, more doctests 
Branch:  u/vdelecroix/26927 (Commits, GitHub, GitLab)  Commit:  a599034fa9a51fdcbe7cba9db9f2bfee33f2bde5 
Dependencies:  Stopgaps: 
Description (last modified by )
Given a quadratic number field, conversion to Python floating point is terribly slow compared to the more complicated MPFI interval floating point conversion
sage: %timeit float(a) 1000 loops, best of 3: 224 µs per loop sage: %timeit RIF(a) 1000000 loops, best of 3: 919 ns per loop
With the branch applied
sage: %timeit float(a) 1000000 loops, best of 3: 245 ns per loop
(see also #26925 for MPFR floating point)
Change History (16)
comment:1 Changed 3 years ago by
 Branch set to u/vdelecroix/26927
 Commit set to 390747a70bb4933620d5e652c1ebcb7559b57e73
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Commit changed from 390747a70bb4933620d5e652c1ebcb7559b57e73 to a599034fa9a51fdcbe7cba9db9f2bfee33f2bde5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a599034  26927: faster float conversion for quad nf elts

comment:3 Changed 3 years ago by
 Description modified (diff)
comment:4 Changed 3 years ago by
I agree it should be fast, but I'm not sure it should be *that* fast. It's not so strange to expect that if they transfer an algebraic number into RealField(100)
, they actually get something on which (close to) 100 digits are correct. That can be quite tricky, especially with high powers of fundamental units, where one conjugate will have large absolute value (easy to compute) and the other has small absolute value. If you compute these by first writing out the unit as a+b*sqrt(d)
and then compute the embedding, it's quite a bit of work to get the precision right (due to a large amount of cancellation). Of course, if you have the element as a power, it's much easier: compute embedding and then take power.
If people need superfast coversions, they should probably have a recipe to do so. I don't think the standard coercions are the place to sacrifice accuracy for speed.
comment:5 followup: ↓ 6 Changed 3 years ago by
I agree with you. But this ticket is *not* about conversion to RealField(100)
but to Python floats. See #29625 for the relevant ticket.
comment:6 in reply to: ↑ 5 Changed 3 years ago by
Replying to vdelecroix:
I agree with you. But this ticket is *not* about conversion to
RealField(100)
but to Python floats. See #29625 for the relevant ticket.
Oh, sorry. The same applies to the 53 bits that float has, though. So I'd suggest to share the same code between the two.
comment:7 Changed 3 years ago by
 Milestone changed from sage8.6 to sage8.7
Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sagepending or sagewishlist.
comment:8 Changed 3 years ago by
 Milestone changed from sage8.7 to sage8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:9 Changed 3 years ago by
 Reviewers set to Daniel Krenn
 Status changed from needs_review to needs_work
 Work issues set to failing doctests, more doctests
Code looks good to me, but there are some failing doctests says the patchbot. I think they are all related to the exception raised in case of complex values.
And, there should be doctests covering the elsebranch of the outer if, and maybe one as well which raises the exception.
comment:10 Changed 3 years ago by
 Milestone changed from sage8.8 to sage8.9
Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).
comment:11 Changed 2 years ago by
 Milestone changed from sage8.9 to sage9.1
Ticket retargeted after milestone closed
comment:12 Changed 22 months ago by
 Milestone changed from sage9.1 to sage9.2
Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.
comment:13 Changed 17 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:14 Changed 11 months ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:15 Changed 6 months ago by
 Milestone changed from sage9.4 to sage9.5
Setting a new milestone for this ticket based on a cursory review.
comment:16 Changed 5 weeks ago by
 Milestone changed from sage9.5 to sage9.6
New commits:
26925: faster float conversion for quad nf elts