Opened 5 years ago
Closed 5 years ago
#24357 closed defect (fixed)
Two bugs in NumberField.composite_fields()
Reported by:  Marc Mezzarobba  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  number fields  Keywords:  
Cc:  Merged in:  
Authors:  Marc Mezzarobba  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  f4eb6e0 (Commits, GitHub, GitLab)  Commit:  f4eb6e0a018779c75c162efdabc9c2cf51c173d5 
Dependencies:  Stopgaps: 
Description
sage: A.<a> = NumberField(x^9  7) ....: B.<b> = NumberField(x^37, embedding=a^3) ....: C.<c> = QuadraticField(1) ....: B.composite_fields(C) ... NameError: global name 'CoercionException' is not defined
sage: y = polygen(QQ, 'y') sage: A.<a> = NumberField(x^3  7, embedding=CC(0.95+1.65*I)) ....: B.<b> = NumberField(y^9  7, embedding=CC(1.16+0.42*I)) ....: A.composite_fields(B) ... PariError: inconsistent variables in polcompositum, x != y
Change History (7)
comment:1 Changed 5 years ago by
Authors:  → Marc Mezzarobba 

Branch:  → u/mmezzarobba/24357composite_fields 
Commit:  → 17c2af487de1e664a03434a4285544b716a23d74 
Status:  new → needs_review 
Summary:  Multiple bugs in NumberField.composite_fields() → Two bugs in NumberField.composite_fields() 
comment:2 Changed 5 years ago by
Commit:  17c2af487de1e664a03434a4285544b716a23d74 → f7801dede8b04a0471b8e367c2eec7128e204bdc 

Branch pushed to git repo; I updated commit sha1. New commits:
f7801de  NumberField.composite_fields(): warn about misfeature

comment:3 followup: 5 Changed 5 years ago by
Some comments:
I've been told it's actually slightly faster to actually import what you need so Python does not have to resolve the extra indirections sage.structure.coerce_exceptions.CoercionException
.
I think it is better to explicitly label which tests are for which tickets.
In the bikeshedding realm, but I don't like removing sage: x = polygen(ZZ)
from TESTS
because I feel like that should be treated like an independent block (it also makes it more clear to the reader [me in this case] what x
is by having it more local).
Otherwise LGTM.
comment:4 Changed 5 years ago by
Commit:  f7801dede8b04a0471b8e367c2eec7128e204bdc → f4eb6e0a018779c75c162efdabc9c2cf51c173d5 

comment:5 Changed 5 years ago by
Thanks for the review!
Replying to tscrim:
I've been told it's actually slightly faster to actually import what you need so Python does not have to resolve the extra indirections
sage.structure.coerce_exceptions.CoercionException
.
Not done, this is slow code in any case...
I think it is better to explicitly label which tests are for which tickets.
Done.
In the bikeshedding realm, but I don't like removing
sage: x = polygen(ZZ)
fromTESTS
because I feel like that should be treated like an independent block (it also makes it more clear to the reader [me in this case] whatx
is by having it more local).
Ok.
comment:6 Changed 5 years ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
Okay. LGTM. Thanks.
comment:7 Changed 5 years ago by
Branch:  u/mmezzarobba/24357composite_fields → f4eb6e0a018779c75c162efdabc9c2cf51c173d5 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Fix two bugs in NumberField.composite_fields()