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:  sage8.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: 
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
 Branch set to u/mmezzarobba/ticket/23117
 Cc zimmerma added
 Commit set to 7718e7f525772ffe172705c62c95f3d65007027f
comment:2 Changed 4 years ago by
 Commit changed from 7718e7f525772ffe172705c62c95f3d65007027f to 7d8cf782d44c9c0a3cf67c83aaa5cb8011d2bee9
comment:3 Changed 4 years ago by
 Status changed from new to needs_review
comment:4 followup: ↓ 6 Changed 4 years ago by
Some comments:
 I don't see the point of the internal
embedding
function. IMO, it makes things more complicated.  This is ugly:
but it might be an necessary evil.

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): 691 691 ... 692 692 TypeError: Attempt to coerce nonintegral RealNumber to Integer 693 693 sage: OG(OG.base_ring().basis()[1]) 694 sqrt5*[1 0]694 (sqrt5)*[1 0] 695 695 [0 1] 696 696 """ 697 697 from sage.rings.ring import is_Ring

Otherwise everything seems to be good.
comment:5 Changed 4 years ago by
 Commit changed from 7d8cf782d44c9c0a3cf67c83aaa5cb8011d2bee9 to a469c7029275e601224f2bd98e6a1820b267554a
Branch pushed to git repo; I updated commit sha1. New commits:
a469c70  set up a real embedding with R[alg] when alg is real

comment:6 in reply to: ↑ 4 ; followup: ↓ 7 Changed 4 years ago by
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 try
s 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 ; followup: ↓ 10 Changed 4 years ago by
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
try
s usingreturn
, 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
 Commit changed from a469c7029275e601224f2bd98e6a1820b267554a to 9b7fac6168a08694d343b7b38bdb850b3c710765
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]

comment:9 Changed 4 years ago by
 Commit changed from 9b7fac6168a08694d343b7b38bdb850b3c710765 to 742c8a4350ce7161a749834a957980a9c3653f8d
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]

comment:10 in reply to: ↑ 7 Changed 4 years ago by
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 dofrom 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
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:12 Changed 4 years ago by
 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:
db64578  Set up embeddings for extensions created with R[alg]

comment:13 Changed 4 years ago by
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
 Status changed from needs_review to positive_review
comment:15 Changed 4 years ago by
 Branch changed from u/mmezzarobba/ticket/23117 to db64578235be03558fe04ac0d10ccd2849abbf29
 Resolution set to fixed
 Status changed from positive_review to closed
(branch not fully tested yet)
New commits:
Honor complex embeddings in conversion ℚ[i] → CIF
Set up embeddings for extensions created with R[alg]