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: sage-9.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:

Status badges

Description (last modified by vdelecroix)

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 vdelecroix

  • Branch set to u/vdelecroix/26927
  • Commit set to 390747a70bb4933620d5e652c1ebcb7559b57e73
  • Status changed from new to needs_review

New commits:

390747a26925: faster float conversion for quad nf elts

comment:2 Changed 3 years ago by git

  • Commit changed from 390747a70bb4933620d5e652c1ebcb7559b57e73 to a599034fa9a51fdcbe7cba9db9f2bfee33f2bde5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a59903426927: faster float conversion for quad nf elts

comment:3 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:4 Changed 3 years ago by nbruin

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 super-fast 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 follow-up: Changed 3 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 3 years ago by nbruin

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 embray

  • Milestone changed from sage-8.6 to sage-8.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 sage-pending or sage-wishlist.

comment:8 Changed 3 years ago by embray

  • Milestone changed from sage-8.7 to sage-8.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 dkrenn

  • 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 else-branch of the outer if, and maybe one as well which raises the exception.

comment:10 Changed 3 years ago by embray

  • Milestone changed from sage-8.8 to sage-8.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 embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:12 Changed 22 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.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 mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:14 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:15 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

comment:16 Changed 5 weeks ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6
Note: See TracTickets for help on using tickets.