Opened 4 years ago
Last modified 9 days ago
#26927 needs_work defect
nf > float conversion should be fast
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  
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 (19)
comment:1 Changed 4 years ago by
Branch:  → u/vdelecroix/26927 

Commit:  → 390747a70bb4933620d5e652c1ebcb7559b57e73 
Status:  new → needs_review 
comment:2 Changed 4 years ago by
Commit:  390747a70bb4933620d5e652c1ebcb7559b57e73 → 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 4 years ago by
Description:  modified (diff) 

comment:4 Changed 4 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 4 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 Changed 4 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 4 years ago by
Milestone:  sage8.6 → 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 4 years ago by
Milestone:  sage8.7 → 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 4 years ago by
Reviewers:  → Daniel Krenn 

Status:  needs_review → needs_work 
Work issues:  → 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 4 years ago by
Milestone:  sage8.8 → 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 3 years ago by
Milestone:  sage8.9 → sage9.1 

Ticket retargeted after milestone closed
comment:12 Changed 3 years ago by
Milestone:  sage9.1 → 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 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:14 Changed 2 years ago by
Milestone:  sage9.3 → sage9.4 

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:15 Changed 19 months ago by
Milestone:  sage9.4 → sage9.5 

Setting a new milestone for this ticket based on a cursory review.
comment:16 Changed 14 months ago by
Milestone:  sage9.5 → sage9.6 

comment:17 Changed 10 months ago by
Milestone:  sage9.6 → sage9.7 

comment:18 Changed 5 months ago by
Milestone:  sage9.7 → sage9.8 

comment:19 Changed 9 days ago by
Milestone:  sage9.8 

New commits:
26925: faster float conversion for quad nf elts