Opened 9 years ago
Closed 8 years ago
#15331 closed defect (fixed)
Do not try to create embedded number field morphisms for non-embedded number fields
Reported by: | SimonKing | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.2 |
Component: | number fields | Keywords: | |
Cc: | jpflori | Merged in: | |
Authors: | Simon King, Marc Mezzarobba, Jean-Pierre Flori | Reviewers: | Marc Mezzarobba, Jean-Pierre Flori |
Report Upstream: | N/A | Work issues: | |
Branch: | 2bedaa1 (Commits, GitHub, GitLab) | Commit: | 2bedaa181f5044f59c90440ca529b1f691e44043 |
Dependencies: | Stopgaps: |
Description
The attempt to create an embedded number field morphisms for non-embedded 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 9 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 9 years ago by
- Commit set to 2123372d5c37cfbabd55fb3f1e63afaa5741ea2a
- Status changed from new to needs_review
comment:3 follow-up: ↓ 6 Changed 8 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 8 years ago by
- Cc jpflori added
comment:5 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:6 in reply to: ↑ 3 ; follow-up: ↓ 7 Changed 8 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 8 years ago by
Hi Jean-Pierre 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 8 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 follow-up: ↓ 11 Changed 8 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, Jean-Pierre 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 remote-tracking branch 'trac/develop' into ticket/15331
|
707ab45 | Added some doc for default ambient field used when trying
|
comment:10 Changed 8 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 8 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 8 years ago by
- Status changed from needs_review to needs_work
comment:13 Changed 8 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 8 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 8 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 8 years ago by
- Status changed from needs_work to needs_review
comment:17 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:18 Changed 8 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: