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

simon-king-jena · 2013-10-26T22:28:35Z

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.

CC: @jpflori

Component: number fields

Author: Simon King, Marc Mezzarobba, Jean-Pierre Flori

Branch/Commit: 2bedaa1

Reviewer: Marc Mezzarobba, Jean-Pierre Flori

Issue created by migration from https://trac.sagemath.org/ticket/15331

The text was updated successfully, but these errors were encountered:

simon-king-jena · 2013-10-27T01:24:59Z

Branch: u/SimonKing/ticket/15331

simon-king-jena · 2013-10-27T01:26:13Z

New commits:

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

simon-king-jena · 2013-10-27T01:26:13Z

Commit: 2123372

simon-king-jena · 2013-10-27T01:26:13Z

Author: Simon King

mezzarobba · 2013-12-19T15:24:18Z

comment:3

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 · 2014-02-27T13:16:26Z

comment:6

Replying to @mezzarobba:

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.

simon-king-jena · 2014-02-27T14:06:12Z

comment:7

Hi Jean-Pierre and Marc,

Replying to @jpflori:

Replying to @mezzarobba:

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 · 2014-03-05T21:45:57Z

comment:8

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 · 2014-03-05T22:05:02Z

Changed branch from u/SimonKing/ticket/15331 to u/jpflori/ticket/15331

jpflori · 2014-03-05T22:05:02Z

Changed author from Simon King to Simon King, Marc Mezzarobba

jpflori · 2014-03-05T22:05:02Z

Reviewer: Marc Mezzarobba, Jean-Pierre Flori

jpflori · 2014-03-05T22:05:02Z

comment:9

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`

jpflori · 2014-03-05T22:05:02Z

Changed commit from 2123372 to 707ab45

sagetrac-git · 2014-03-05T22:09:28Z

Changed commit from 707ab45 to d84620d

sagetrac-git · 2014-03-05T22:09:28Z

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

`d84620d`	`Remove useless import of pushout in number_field.py.`

mezzarobba · 2014-03-07T09:46:52Z

comment:11

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 · 2014-03-07T09:50:40Z

comment:13

Replying to @mezzarobba:

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.

sagetrac-git · 2014-03-12T15:54:45Z

Changed commit from d84620d to 1a3f9c2

sagetrac-git · 2014-03-12T15:54:45Z

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

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

sagetrac-git · 2014-03-12T16:04:16Z

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

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

sagetrac-git · 2014-03-12T16:04:16Z

Changed commit from 1a3f9c2 to 2bedaa1

mezzarobba · 2014-03-12T17:17:00Z

Changed author from Simon King, Marc Mezzarobba to Simon King, Marc Mezzarobba, Jean-Pierre Flori

vbraun · 2014-03-21T17:47:32Z

Changed branch from u/jpflori/ticket/15331 to 2bedaa1

simon-king-jena added this to the sage-6.1 milestone Oct 26, 2013

simon-king-jena added c: number fields labels Oct 26, 2013

simon-king-jena added the s: needs review label Oct 27, 2013

mezzarobba added s: needs info and removed s: needs review labels Dec 19, 2013

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014

jpflori added s: needs review and removed s: needs info labels Mar 5, 2014

mezzarobba added s: needs work and removed s: needs review labels Mar 7, 2014

jpflori added s: needs review and removed s: needs work labels Mar 12, 2014

mezzarobba added s: positive review and removed s: needs review labels Mar 12, 2014

vbraun removed the s: positive review label Mar 21, 2014

vbraun closed this as completed in ebd2c00 Mar 21, 2014

nbruin mentioned this issue Dec 29, 2022

Coercion discovery fails to be transitive #15303

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

simon-king-jena commented Oct 26, 2013

simon-king-jena commented Oct 27, 2013

simon-king-jena commented Oct 27, 2013

simon-king-jena commented Oct 27, 2013

simon-king-jena commented Oct 27, 2013

mezzarobba commented Dec 19, 2013

jpflori commented Feb 27, 2014

simon-king-jena commented Feb 27, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

sagetrac-git mannequin commented Mar 5, 2014

sagetrac-git mannequin commented Mar 5, 2014

mezzarobba commented Mar 7, 2014

jpflori commented Mar 7, 2014

sagetrac-git mannequin commented Mar 12, 2014

sagetrac-git mannequin commented Mar 12, 2014

sagetrac-git mannequin commented Mar 12, 2014

sagetrac-git mannequin commented Mar 12, 2014

mezzarobba commented Mar 12, 2014

vbraun commented Mar 21, 2014

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

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

Comments

simon-king-jena commented Oct 26, 2013

simon-king-jena commented Oct 27, 2013

simon-king-jena commented Oct 27, 2013

simon-king-jena commented Oct 27, 2013

simon-king-jena commented Oct 27, 2013

mezzarobba commented Dec 19, 2013

jpflori commented Feb 27, 2014

simon-king-jena commented Feb 27, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

jpflori commented Mar 5, 2014

sagetrac-git mannequin commented Mar 5, 2014

sagetrac-git mannequin commented Mar 5, 2014

mezzarobba commented Mar 7, 2014

jpflori commented Mar 7, 2014

sagetrac-git mannequin commented Mar 12, 2014

sagetrac-git mannequin commented Mar 12, 2014

sagetrac-git mannequin commented Mar 12, 2014

sagetrac-git mannequin commented Mar 12, 2014

mezzarobba commented Mar 12, 2014

vbraun commented Mar 21, 2014