Set up embeddings for extensions created using the syntax R[alg]

[mezzarobba]

This fixes in particular the following issue, found thanks to a question
of Paul Zimmermann:

sage: QQ[I](I.pyobject())
...
TypeError: No compatible natural embeddings found for Number Field in I
with defining polynomial x^2 + 1 and Complex Lazy Field

Also fix the conversion of elements of ℚ[i] to CIF to correctly take into account the choice of embedding.

CC: @zimmermann6

Component: algebra

Author: Marc Mezzarobba

Branch/Commit: db64578

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/23117

[mezzarobba]

Author: Marc Mezzarobba

[mezzarobba]

Commit: 7718e7f

[mezzarobba]

Branch: u/mmezzarobba/ticket/23117

[mezzarobba]

comment:1

(branch not fully tested yet)

---

New commits:

933a711	Honor complex embeddings in conversion ℚ[i] → CIF
7718e7f	Set up embeddings for extensions created with R[alg]

[sagetrac-git]

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

8c8d474	Honor complex embeddings in conversion ℚ[i] → CIF
7d8cf78	Set up embeddings for extensions created with R[alg]

[sagetrac-git]

Changed commit from 7718e7f to 7d8cf78

[tscrim]

comment:4

Some comments:

• I don't see the point of the internal embedding function. IMO, it makes things more complicated.

• This is ugly:

  diff --git a/src/sage/algebras/group_algebra.py b/src/sage/algebras/group_algebra.py
  index d275ee1..f1aa4ff 100644
  --- a/src/sage/algebras/group_algebra.py
  +++ b/src/sage/algebras/group_algebra.py
  @@ -691,7 +691,7 @@ class GroupAlgebra(CombinatorialFreeModule):
               ...
               TypeError: Attempt to coerce non-integral RealNumber to Integer
               sage: OG(OG.base_ring().basis()[1])
  -            sqrt5*[1 0]
  +            -(-sqrt5)*[1 0]
               [0 1]
           """
           from sage.rings.ring import is_Ring

  but it might be an necessary evil.

Otherwise everything seems to be good.

[sagetrac-git]

Changed commit from 7d8cf78 to a469c70

[sagetrac-git]

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

a469c70

set up a real embedding with R[alg] when alg is real

[mezzarobba]

comment:6

Thanks for your comments!

Replying to @tscrim:

• I don't see the point of the internal embedding function. IMO, it makes things more complicated.

Having an internal function makes it easy to exit at any point of the chain of trys using return, that's all.

• This is ugly: [...]
  but it might be an necessary evil.

That's what I thought, but I had another look thanks to your comment, and it turns out it was a weakness in my code (or perhaps in NumberField/NumberFieldElement_quadratic, which are not clever enough to recognize the standard embedding of a real number field when given as an embedding into ℂ).

[tscrim]

comment:7

Replying to @mezzarobba:

Thanks for your comments!

Replying to @tscrim:

• I don't see the point of the internal embedding function. IMO, it makes things more complicated.

Having an internal function makes it easy to exit at any point of the chain of trys using return, that's all.

At least right now you don't really have a chain (and your current order is suboptimal when it fails for CIF). I would just do

from sage.rings.all import CIF, CLF, RIF, RLF
try:
    iv = CIF(elt)
except (TypeError, ValueError):
    emb = None
else:
    try:
        RIF(elt) # There is a better check for realness, correct?
        LF = RLF
    except (TypeError, ValueError):
        LF = CLF
    # First try creating an ANRoot manually, because extension(...,
    # embedding=CLF(expr)) (or ...QQbar(expr)) would normalize the
    # expression in QQbar, which currently is VERY slow in many
    # cases. This may fail when minpoly has close roots or elt is a
    # complicated symbolic expression.
    # TODO: Rewrite using #19362 and/or #17886 and/or #15600 once
    # those issues are solved.
    from sage.rings.qqbar import AlgebraicNumber, ANRoot
    try:
        elt = AlgebraicNumber(ANRoot(minpoly, iv))
    except ValueError:
        pass
    emb = LF(elt)

• This is ugly: [...]
  but it might be an necessary evil.

That's what I thought, but I had another look thanks to your comment, and it turns out it was a weakness in my code (or perhaps in NumberField/NumberFieldElement_quadratic, which are not clever enough to recognize the standard embedding of a real number field when given as an embedding into ℂ).

So it is a necessary evil at present until another part of code is improved? Am I understanding your comment correctly?

[sagetrac-git]

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

9b7fac6

Set up embeddings for extensions created with R[alg]

[sagetrac-git]

Changed commit from a469c70 to 9b7fac6

[sagetrac-git]

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

742c8a4

Set up embeddings for extensions created with R[alg]

[sagetrac-git]

Changed commit from 9b7fac6 to 742c8a4

[mezzarobba]

comment:10

Replying to @tscrim:

At least right now you don't really have a chain (and your current order is suboptimal when it fails for CIF). I would just do

from sage.rings.all import CIF, CLF, RIF, RLF
try:
    iv = CIF(elt)
except (TypeError, ValueError):
    emb = None
else:
    ...

Why not—I changed the implementation to something like that.

    try:
        RIF(elt) # There is a better check for realness, correct?

I don't know. I'm now testing

if iv.imag().is_zero() or iv.imag().contains_zero() and elt.imag().is_zero())

but I don't think it makes a significant difference.

That's what I thought, but I had another look thanks to your comment, and it turns out it was a weakness in my code (or perhaps in NumberField/NumberFieldElement_quadratic, which are not clever enough to recognize the standard embedding of a real number field when given as an embedding into ℂ).

So it is a necessary evil at present until another part of code is improved? Am I understanding your comment correctly?

No, sorry if I was not clear: it was a weakness of my initial implementation, fixed in a469c70 (itself now squashed into 742c8a4).

[tscrim]

comment:11

Thanks. LGTM.

[tscrim]

Reviewer: Travis Scrimshaw

[sagetrac-git]

Changed commit from 742c8a4 to db64578

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

db64578

Set up embeddings for extensions created with R[alg]

[mezzarobba]

comment:13

As it turns out, some of the doctest changes were no longer necessary (nor correct!) with the new version that sets up real embeddings when possible.

[vbraun]

Changed branch from u/mmezzarobba/ticket/23117 to db64578