Opened 4 years ago
Closed 4 years ago
#24357 closed defect (fixed)
Two bugs in NumberField.composite_fields()
Reported by:  mmezzarobba  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 4 years ago by
 Branch set to u/mmezzarobba/24357composite_fields
 Commit set to 17c2af487de1e664a03434a4285544b716a23d74
 Status changed from new to needs_review
 Summary changed from Multiple bugs in NumberField.composite_fields() to Two bugs in NumberField.composite_fields()
comment:2 Changed 4 years ago by
 Commit changed from 17c2af487de1e664a03434a4285544b716a23d74 to f7801dede8b04a0471b8e367c2eec7128e204bdc
Branch pushed to git repo; I updated commit sha1. New commits:
f7801de  NumberField.composite_fields(): warn about misfeature

comment:3 followup: ↓ 5 Changed 4 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 4 years ago by
 Commit changed from f7801dede8b04a0471b8e367c2eec7128e204bdc to f4eb6e0a018779c75c162efdabc9c2cf51c173d5
comment:5 in reply to: ↑ 3 Changed 4 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 4 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Okay. LGTM. Thanks.
comment:7 Changed 4 years ago by
 Branch changed from u/mmezzarobba/24357composite_fields to f4eb6e0a018779c75c162efdabc9c2cf51c173d5
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Fix two bugs in NumberField.composite_fields()