Opened 6 years ago

Closed 6 years ago

#20400 closed defect (fixed)

Conversion NumberField -> QQbar should always work for rationals

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-7.2
Component: number fields Keywords:
Cc: mmezzarobba Merged in:
Authors: Vincent Delecroix Reviewers: Thierry Monteil
Report Upstream: N/A Work issues:
Branch: 8d8602f (Commits, GitHub, GitLab) Commit: 8d8602f93e38dd7fa9e8d0a8502d5454a9988d4c
Dependencies: Stopgaps:

Status badges

Description (last modified by vdelecroix)

After #14485 we have

sage: NF.<alpha> = NumberField(x^5 + 7*x + 3)
sage: QQbar(alpha**5 + 7*alpha)
Traceback (most recent call last):
ValueError: need a real or complex embedding to convert number field
element to algebraic number

but rationals are non ambiguous with respect to their embeddings.

See also the related #4621

Change History (8)

comment:1 Changed 6 years ago by vdelecroix

  • Description modified (diff)

I propose

@@ -2373,8 +2374,7 @@ cdef class NumberFieldElement(FieldElement):
         emb = self._parent.coerce_embedding()
         if emb is None:
-            raise ValueError("need a real or complex embedding to convert "
-                             "number field element to algebraic number")
+            return parent(self._rational_())

And also to wait for the next beta...

comment:2 Changed 6 years ago by tmonteil

I think that you should still have a try except statement with an adapted message.

Otherwise the error message will be:

TypeError: Unable to coerce alpha to a rational

which might end up as "Uh, i know that it is not a rational, i was asking for an algebraic !".

comment:3 Changed 6 years ago by vdelecroix

  • Branch set to u/vdelecroix/20400
  • Dependencies #14485 deleted
  • Status changed from new to needs_review

comment:4 Changed 6 years ago by git

  • Commit set to 8d8602f93e38dd7fa9e8d0a8502d5454a9988d4c

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

8d8602fTrac 20400: conversion rational in NF -> QQbar

comment:5 Changed 6 years ago by tmonteil

  • Reviewers set to Thierry Monteil

Looks good to me, i kicked the patchbot to see if some problem appears elsewhere.

comment:6 Changed 6 years ago by vdelecroix

Patchbot is happy!

comment:7 Changed 6 years ago by tmonteil

  • Status changed from needs_review to positive_review

comment:8 Changed 6 years ago by vbraun

  • Branch changed from u/vdelecroix/20400 to 8d8602f93e38dd7fa9e8d0a8502d5454a9988d4c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.