Opened 6 years ago

Closed 5 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) 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 6 years ago by SimonKing

  • 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 6 years ago by SimonKing

  • Authors set to Simon King
  • Commit set to 2123372d5c37cfbabd55fb3f1e63afaa5741ea2a
  • Status changed from new to needs_review

New commits:

[changeset:2123372]Raise type error for number field morphisms of non-embedded number fields

comment:3 follow-up: Changed 6 years ago by mmezzarobba

  • 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 of number_field.py necessary?
  • shouldn't
                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)
    
    read
                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 jpflori

  • Cc jpflori added

comment:5 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:6 in reply to: ↑ 3 ; follow-up: Changed 5 years ago by 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 of number_field.py necessary?
  • shouldn't
                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)
    
    read
                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 5 years ago by SimonKing

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 of number_field.py necessary?

Sorry, I don't recall, and at the moment I am too much occupied with other things.

  • shouldn't
                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)
    
    read
                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 5 years ago by jpflori

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: Changed 5 years ago by jpflori

  • Authors changed from Simon King to Simon King, Marc Mezzarobba
  • 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:

8798dd9Simplify the logic for finding coercions between embedded number fields
fb1cfbcMerge remote-tracking branch 'trac/develop' into ticket/15331
707ab45Added some doc for default ambient field used when trying

comment:10 Changed 5 years ago by git

  • Commit changed from 707ab4554dae0b00d7e3542cb417993d8503ab4e to d84620d36226927a6e1a13aad98baf8b999daf3c

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

d84620dRemove useless import of pushout in number_field.py.

comment:11 in reply to: ↑ 9 Changed 5 years ago by 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.

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 5 years ago by mmezzarobba

  • Status changed from needs_review to needs_work

comment:13 Changed 5 years ago by jpflori

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 5 years ago by git

  • Commit changed from d84620d36226927a6e1a13aad98baf8b999daf3c to 1a3f9c2a3f5a6af95ea59870b272a770877ee98a

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

1a3f9c2Test the algebraic closure before CC when constructing embeddings of number fields.

comment:15 Changed 5 years ago by git

  • Commit changed from 1a3f9c2a3f5a6af95ea59870b272a770877ee98a to 2bedaa181f5044f59c90440ca529b1f691e44043

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

2bedaa1Do not forget to define ambient_field before computing its algebraic closure.

comment:16 Changed 5 years ago by jpflori

  • Status changed from needs_work to needs_review

comment:17 Changed 5 years ago by mmezzarobba

  • Authors changed from Simon King, Marc Mezzarobba to Simon King, Marc Mezzarobba, Jean-Pierre Flori
  • Status changed from needs_review to positive_review

comment:18 Changed 5 years ago by vbraun

  • Branch changed from u/jpflori/ticket/15331 to 2bedaa181f5044f59c90440ca529b1f691e44043
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.