Do not try to create embedded number field morphisms for non-embedded number fields

[SimonKing]

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.

[SimonKing]

New commits:

[changeset:2123372]

Raise type error for number field morphisms of non-embedded number fields

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

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

[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

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

[jpflori]

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

[git]

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

​d84620d

Remove useless import of pushout in number_field.py.

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

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

[git]

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

​1a3f9c2

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

[git]

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

​2bedaa1

Do not forget to define ambient_field before computing its algebraic closure.