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: sage-8.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:

Status badges

Description

sage: A.<a> = NumberField(x^9 - 7)
....: B.<b> = NumberField(x^3-7, 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 mmezzarobba

  • Authors set to Marc Mezzarobba
  • Branch set to u/mmezzarobba/24357-composite_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()

New commits:

17c2af4Fix two bugs in NumberField.composite_fields()

comment:2 Changed 4 years ago by git

  • Commit changed from 17c2af487de1e664a03434a4285544b716a23d74 to f7801dede8b04a0471b8e367c2eec7128e204bdc

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

f7801deNumberField.composite_fields(): warn about misfeature

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

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 git

  • Commit changed from f7801dede8b04a0471b8e367c2eec7128e204bdc to f4eb6e0a018779c75c162efdabc9c2cf51c173d5

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

66a1e6cFix two bugs in NumberField.composite_fields()
f4eb6e0NumberField.composite_fields(): warn about misfeature

comment:5 in reply to: ↑ 3 Changed 4 years ago by mmezzarobba

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

Ok.

comment:6 Changed 4 years ago by tscrim

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

Okay. LGTM. Thanks.

comment:7 Changed 4 years ago by vbraun

  • Branch changed from u/mmezzarobba/24357-composite_fields to f4eb6e0a018779c75c162efdabc9c2cf51c173d5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.