Opened 7 years ago
Closed 6 years ago
#15331 closed defect (fixed)
Do not try to create embedded number field morphisms for nonembedded number fields
Reported by:  SimonKing  Owned by:  

Priority:  major  Milestone:  sage6.2 
Component:  number fields  Keywords:  
Cc:  jpflori  Merged in:  
Authors:  Simon King, Marc Mezzarobba, JeanPierre Flori  Reviewers:  Marc Mezzarobba, JeanPierre Flori 
Report Upstream:  N/A  Work issues:  
Branch:  2bedaa1 (Commits)  Commit:  2bedaa181f5044f59c90440ca529b1f691e44043 
Dependencies:  Stopgaps: 
Description
The attempt to create an embedded number field morphisms for nonembedded number fields currently fails (and should of course fail).
sage: L.<i> = NumberField(x^2 + 1) sage: K = NumberField(L(i/2+3).minpoly(), names=('i0',), embedding=L(i/2+3)) sage: from sage.rings.number_field import number_field_morphisms sage: number_field_morphisms.EmbeddedNumberFieldMorphism(R, self) Traceback (most recent call last): ... RuntimeError: maximum recursion depth exceeded in __instancecheck__
However, instead of running into an infinite recursion, a quick and simple ValueError
(or perhaps TypeError
) should be raised.
Change History (18)
comment:1 Changed 7 years ago by
 Branch set to u/SimonKing/ticket/15331
 Created changed from 10/26/13 22:28:35 to 10/26/13 22:28:35
 Modified changed from 10/26/13 22:28:35 to 10/26/13 22:28:35
comment:2 Changed 7 years ago by
 Commit set to 2123372d5c37cfbabd55fb3f1e63afaa5741ea2a
 Status changed from new to needs_review
comment:3 followup: ↓ 6 Changed 6 years ago by
 Status changed from needs_review to needs_info
Hi Simon,
I'm trying to review this ticket, but there are a couple of things I don't understand:
 why can't
EmbeddedNumberFieldMorphism
try using the algebraic closure by itself?  even if it really can't, is the giant
except
clause on line 6194 ofnumber_field.py
necessary?  shouldn't
read
Lemb = Lemb.codomain() # number_field_morphisms.pyx:187 while Lemb.coerce_embedding() is not None: Lemb = Lemb.coerce_embedding().codomain() ambient_field = pushout(Kemb, Lemb)
?Lemb = Lemb.codomain() while Lemb.coerce_embedding() is not None: Lemb = Lemb.coerce_embedding().codomain() ambient_field = pushout(Kemb, Lemb)
Sorry if these are stupid questions, I'm still trying to learn how this all works!
My attempt at simplifying the code based on these remarks is at u/mmezzarobba/embedded_NF_morphisms
.
comment:4 Changed 6 years ago by
 Cc jpflori added
comment:5 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:6 in reply to: ↑ 3 ; followup: ↓ 7 Changed 6 years ago by
Replying to mmezzarobba:
Hi Simon,
I'm trying to review this ticket, but there are a couple of things I don't understand:
 why can't
EmbeddedNumberFieldMorphism
try using the algebraic closure by itself? even if it really can't, is the giant
except
clause on line 6194 ofnumber_field.py
necessary? shouldn't
readLemb = Lemb.codomain() # number_field_morphisms.pyx:187 while Lemb.coerce_embedding() is not None: Lemb = Lemb.coerce_embedding().codomain() ambient_field = pushout(Kemb, Lemb)?Lemb = Lemb.codomain() while Lemb.coerce_embedding() is not None: Lemb = Lemb.coerce_embedding().codomain() ambient_field = pushout(Kemb, Lemb)Sorry if these are stupid questions, I'm still trying to learn how this all works!
My attempt at simplifying the code based on these remarks is at
u/mmezzarobba/embedded_NF_morphisms
.
Any remark on this Simon?
If you're too busy, I'll just have a look for myself and try to take some action to get this merged.
comment:7 in reply to: ↑ 6 Changed 6 years ago by
Hi JeanPierre and Marc,
Replying to jpflori:
Replying to mmezzarobba:
Hi Simon,
I'm trying to review this ticket, but there are a couple of things I don't understand:
 why can't
EmbeddedNumberFieldMorphism
try using the algebraic closure by itself? even if it really can't, is the giant
except
clause on line 6194 ofnumber_field.py
necessary?
Sorry, I don't recall, and at the moment I am too much occupied with other things.
 shouldn't
readLemb = Lemb.codomain() # number_field_morphisms.pyx:187 while Lemb.coerce_embedding() is not None: Lemb = Lemb.coerce_embedding().codomain() ambient_field = pushout(Kemb, Lemb)Lemb = Lemb.codomain() while Lemb.coerce_embedding() is not None: Lemb = Lemb.coerce_embedding().codomain() ambient_field = pushout(Kemb, Lemb)
Yes, unless I wanted to see if the pushout exists in each step (catching the error if it does not exist).
Sorry if these are stupid questions, I'm still trying to learn how this all works!
So do I (now) ;)
My attempt at simplifying the code based on these remarks is at
u/mmezzarobba/embedded_NF_morphisms
.Any remark on this Simon?
If you're too busy,
Sorry, I am.
Best regards,
Simon
comment:8 Changed 6 years ago by
I'm ok with Marc changes.
My only concern is that it might be the case that previously catched errors are not anymore, only ValueError are. Not sure how it could break the coercion system as before such errors would just lead to returning nothing and now errors might be raised.
comment:9 followup: ↓ 11 Changed 6 years ago by
 Branch changed from u/SimonKing/ticket/15331 to u/jpflori/ticket/15331
 Commit changed from 2123372d5c37cfbabd55fb3f1e63afaa5741ea2a to 707ab4554dae0b00d7e3542cb417993d8503ab4e
 Reviewers set to Marc Mezzarobba, JeanPierre Flori
 Status changed from needs_info to needs_review
I've just added some doc about which ambient field is tried by default.
If nobody feels concerned as I do about the uncatched errors (maybe the coercion framework already deals with them in a correct way, it just too late to try to remember about that), the let's get this merged.
New commits:
8798dd9  Simplify the logic for finding coercions between embedded number fields

fb1cfbc  Merge remotetracking branch 'trac/develop' into ticket/15331

707ab45  Added some doc for default ambient field used when trying

comment:10 Changed 6 years ago by
 Commit changed from 707ab4554dae0b00d7e3542cb417993d8503ab4e to d84620d36226927a6e1a13aad98baf8b999daf3c
Branch pushed to git repo; I updated commit sha1. New commits:
d84620d  Remove useless import of pushout in number_field.py.

comment:11 in reply to: ↑ 9 Changed 6 years ago by
Replying to jpflori:
I've just added some doc about which ambient field is tried by default.
Hmm, CDF
is actually tried before the algebraic closure of the pushout, contrary to what the description you wrote suggests. (That's what Simon's patch was doing, and I tried to leave the results of successful calls unchanged in mine.) I think we should either swap the corresponding items in the docstring, or try the algebraic closure first if this order makes more sense.
If nobody feels concerned as I do about the uncatched errors (maybe the coercion framework already deals with them in a correct way, it just too late to try to remember about that), the let's get this merged.
Since no one seems to know in what scenarios these except clauses are supposed to be used (and the patches + tickets they come from do not make it clear to me), I thought we could remove them for now and add them back later, with tests triggering them, if necessary. But please feel free to change back the code to catch more exceptions if you disagree.
comment:12 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:13 Changed 6 years ago by
Replying to mmezzarobba:
Replying to jpflori:
I've just added some doc about which ambient field is tried by default.
Hmm,
CDF
is actually tried before the algebraic closure of the pushout, contrary to what the description you wrote suggests. (That's what Simon's patch was doing, and I tried to leave the results of successful calls unchanged in mine.) I think we should either swap the corresponding items in the docstring, or try the algebraic closure first if this order makes more sense.
Oops, in fact, I took back the description with what I built on top of the original Simon branch, and there I tried CDF last. It seemed to make more sense to me to test it after the algebraic closure which a priori is smaller. So I'd be in favor of changing the current behavior and not the doc.
If nobody feels concerned as I do about the uncatched errors (maybe the coercion framework already deals with them in a correct way, it just too late to try to remember about that), the let's get this merged.
Since no one seems to know in what scenarios these except clauses are supposed to be used (and the patches + tickets they come from do not make it clear to me), I thought we could remove them for now and add them back later, with tests triggering them, if necessary. But please feel free to change back the code to catch more exceptions if you disagree.
I agree with that plan.
comment:14 Changed 6 years ago by
 Commit changed from d84620d36226927a6e1a13aad98baf8b999daf3c to 1a3f9c2a3f5a6af95ea59870b272a770877ee98a
Branch pushed to git repo; I updated commit sha1. New commits:
1a3f9c2  Test the algebraic closure before CC when constructing embeddings of number fields.

comment:15 Changed 6 years ago by
 Commit changed from 1a3f9c2a3f5a6af95ea59870b272a770877ee98a to 2bedaa181f5044f59c90440ca529b1f691e44043
Branch pushed to git repo; I updated commit sha1. New commits:
2bedaa1  Do not forget to define ambient_field before computing its algebraic closure.

comment:16 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:17 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:18 Changed 6 years ago by
 Branch changed from u/jpflori/ticket/15331 to 2bedaa181f5044f59c90440ca529b1f691e44043
 Resolution set to fixed
 Status changed from positive_review to closed
New commits: