Opened 5 years ago

Closed 5 years ago

#17495 closed defect (fixed)

Fix refine_embedding when some but not all embeddings are real

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.5
Component: number fields Keywords:
Cc: Merged in:
Authors: Martin von Gagern Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 8aac6fa (Commits) Commit: 8aac6fac1c438fca5defd9b4418c0ea0c541f7ea
Dependencies: Stopgaps:

Description

The number field below has one real embedding and two complex embeddings. These complex embeddings should be ignored in refine_embedding, but that's not the case, causing

sage: K.<a> = NumberField(x^3 + x - 1, embedding=0.68)
sage: from sage.rings.number_field.number_field import refine_embedding
sage: refine_embedding(K.specified_complex_embedding(), 200)
Traceback (most recent call last):
...
TypeError: Unable to convert number to real interval.

Change History (11)

comment:1 Changed 5 years ago by gagern

  • Branch set to u/gagern/ticket/17495
  • Created changed from 12/13/14 13:22:26 to 12/13/14 13:22:26
  • Modified changed from 12/13/14 13:22:26 to 12/13/14 13:22:26

comment:2 Changed 5 years ago by gagern

  • Authors set to Martin von Gagern
  • Commit set to f30d693c593fcf8de8500c6821f2beff4c56c3a4
  • Status changed from new to needs_review

New commits:

f30d693Only consider real embeddings if old embedding is into real lazy field.

comment:3 Changed 5 years ago by git

  • Commit changed from f30d693c593fcf8de8500c6821f2beff4c56c3a4 to cbe823a5dc0f0d31ada9afbf4003c386d964e221

Branch pushed to git repo; I updated commit sha1. New commits:

cbe823aWhen determining embeddings, leave coefficients rational.

comment:4 Changed 5 years ago by gagern

Regarding my second commit:

sage: f = QQ[x](69721504*x^8 + 251777664*x^6 + 329532012*x^4 + 184429548*x^2 + 37344321)
sage: %timeit CDF[x](f).roots()
1000 loops, best of 3: 525 µs per loop
sage: %timeit f.roots(CDF)
1000 loops, best of 3: 465 µs per loop
sage: %timeit QQbar[x](f).roots()
10 loops, best of 3: 24.4 ms per loop
sage: %timeit f.roots(QQbar)
10 loops, best of 3: 20.8 ms per loop

So using f.roots(K) should be better than K['x'](f) in pretty much all cases.

comment:5 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Please make this two lines:

r = f.roots(K, False); r.sort()

comment:6 Changed 5 years ago by jdemeyer

and use f.roots(K, multiplicities=False), which is much more explicit.

comment:7 Changed 5 years ago by jdemeyer

For the example you added, could you do it also for prec=Infinity?

comment:8 Changed 5 years ago by git

  • Commit changed from cbe823a5dc0f0d31ada9afbf4003c386d964e221 to 8aac6fac1c438fca5defd9b4418c0ea0c541f7ea

Branch pushed to git repo; I updated commit sha1. New commits:

8aac6faMinor improvements regarding number field embeddings.

comment:9 Changed 5 years ago by gagern

  • Status changed from needs_work to needs_review

comment:10 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:11 Changed 5 years ago by vbraun

  • Branch changed from u/gagern/ticket/17495 to 8aac6fac1c438fca5defd9b4418c0ea0c541f7ea
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.