Opened 4 years ago

Closed 4 years ago

#23117 closed defect (fixed)

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

Reported by: mmezzarobba Owned by:
Priority: major Milestone: sage-8.0
Component: algebra Keywords:
Cc: zimmerma Merged in:
Authors: Marc Mezzarobba Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: db64578 (Commits, GitHub, GitLab) Commit: db64578235be03558fe04ac0d10ccd2849abbf29
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (15)

comment:1 Changed 4 years ago by mmezzarobba

  • Authors set to Marc Mezzarobba
  • Branch set to u/mmezzarobba/ticket/23117
  • Cc zimmerma added
  • Commit set to 7718e7f525772ffe172705c62c95f3d65007027f

(branch not fully tested yet)


New commits:

933a711Honor complex embeddings in conversion ℚ[i] → CIF
7718e7fSet up embeddings for extensions created with R[alg]

comment:2 Changed 4 years ago by git

  • Commit changed from 7718e7f525772ffe172705c62c95f3d65007027f to 7d8cf782d44c9c0a3cf67c83aaa5cb8011d2bee9

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

8c8d474Honor complex embeddings in conversion ℚ[i] → CIF
7d8cf78Set up embeddings for extensions created with R[alg]

comment:3 Changed 4 years ago by mmezzarobba

  • Status changed from new to needs_review

comment:4 follow-up: Changed 4 years ago by tscrim

Some comments:

  • I don't see the point of the internal embedding function. IMO, it makes things more complicated.
  • This is ugly:
    • src/sage/algebras/group_algebra.py

      diff --git a/src/sage/algebras/group_algebra.py b/src/sage/algebras/group_algebra.py
      index d275ee1..f1aa4ff 100644
      a b class GroupAlgebra(CombinatorialFreeModule): 
      691691            ...
      692692            TypeError: Attempt to coerce non-integral RealNumber to Integer
      693693            sage: OG(OG.base_ring().basis()[1])
      694             sqrt5*[1 0]
       694            -(-sqrt5)*[1 0]
      695695            [0 1]
      696696        """
      697697        from sage.rings.ring import is_Ring
    but it might be an necessary evil.

Otherwise everything seems to be good.

comment:5 Changed 4 years ago by git

  • Commit changed from 7d8cf782d44c9c0a3cf67c83aaa5cb8011d2bee9 to a469c7029275e601224f2bd98e6a1820b267554a

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

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

comment:6 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by mmezzarobba

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 ℂ).

comment:7 in reply to: ↑ 6 ; follow-up: Changed 4 years ago by tscrim

Replying to mmezzarobba:

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?

comment:8 Changed 4 years ago by git

  • Commit changed from a469c7029275e601224f2bd98e6a1820b267554a to 9b7fac6168a08694d343b7b38bdb850b3c710765

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

9b7fac6Set up embeddings for extensions created with R[alg]

comment:9 Changed 4 years ago by git

  • Commit changed from 9b7fac6168a08694d343b7b38bdb850b3c710765 to 742c8a4350ce7161a749834a957980a9c3653f8d

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

742c8a4Set up embeddings for extensions created with R[alg]

comment:10 in reply to: ↑ 7 Changed 4 years ago by mmezzarobba

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).

comment:11 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thanks. LGTM.

comment:12 Changed 4 years ago by git

  • Commit changed from 742c8a4350ce7161a749834a957980a9c3653f8d to db64578235be03558fe04ac0d10ccd2849abbf29
  • Status changed from positive_review to needs_review

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

db64578Set up embeddings for extensions created with R[alg]

comment:13 Changed 4 years ago by mmezzarobba

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.

comment:14 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:15 Changed 4 years ago by vbraun

  • Branch changed from u/mmezzarobba/ticket/23117 to db64578235be03558fe04ac0d10ccd2849abbf29
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.