When a parent is equipped with an embedding, consider coercions that don't go through the embedding

[mezzarobba]

Parent.discover_coerce_map_from used to always give priority to coercions that start with applying an embedding over those provided by _coerce_map_from_. This leads to coercion failures such as the following:

sage: K.<a> = NumberField(x^2+1/2, embedding=CC(0,1))
sage: L = NumberField(x^2+2, 'b', embedding=1/a)     
sage: R = PolynomialRing(L, 'x')                     
sage: R.coerce_map_from(R.base_ring())               
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-8-fb89bb074d92> in <module>()
----> 1 R.coerce_map_from(R.base_ring())
...
AttributeError: 'NoneType' object has no attribute 'domain'

This patch series modifies the coercion discovery algorithm to fix the issue, and fixes or works around a few weaknesses in existing code exposed by the change. See the individual commit messages for details.

CC: @robertwb @simon-king-jena @jpflori

Component: coercion

Keywords: embedding

Author: Marc Mezzarobba, Vincent Delecroix

Branch/Commit: a0ac11b

Reviewer: Vincent Delecroix

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

[mezzarobba]

comment:1

Apparently, the problem is more general stems from the policy of Parent.discover_coerce_map_from of always giving priority to coercions that start with applying an embedding over those provided by _coerce_map_from_.

In the special case of this ticket, a workaround would be to revert part of a3c5958b8e6983a3ef8466d0bd0b6ce09fd32d41 so that the BaseringInjection gets registered as early as possible:

--- a/src/sage/rings/polynomial/polynomial_ring.py
+++ b/src/sage/rings/polynomial/polynomial_ring.py
@@ -283,9 +283,11 @@ class PolynomialRing_general(sage.algebras.algebra.Algebra):
         # since we want to use PolynomialBaseringInjection.
         sage.algebras.algebra.Algebra.__init__(self, base_ring, names=name, normalize=True, category=category)
         self.__generator = self._polynomial_class(self, [0,1], is_gen=True)
+        base_inject = PolynomialBaseringInjection(base_ring, self)
+        self._unset_coercions_used()
         self._populate_coercion_lists_(
-                #coerce_list = [base_inject],
-                #convert_list = [list, base_inject],
+                coerce_list = [base_inject],
+                convert_list = [list, base_inject],
                 convert_method_name = '_polynomial_')

@@ -552,8 +554,8 @@ class PolynomialRing_general(sage.algebras.algebra.Algebra):
         """
         # In the first place, handle the base ring
         base_ring = self.base_ring()
-        if P is base_ring:
-            return PolynomialBaseringInjection(base_ring, self)
+        #if P is base_ring:
+        #    return PolynomialBaseringInjection(base_ring, self)
         # handle constants that canonically coerce into self.base_ring()
         # first, if possible
         try:

(I didn't check whether this breaks anything else.)

But the same issue potentially affects anything constructed over a base ring with coerce_embedding. At best, a suboptimal coercion is found:

sage: M = MatrixSpace(L, 2, 2)
sage: M.coerce_map_from(L)
Composite map:
  From: Number Field in b with defining polynomial x^2 + 2
  To:   Full MatrixSpace of 2 by 2 dense matrices over Number Field in b with defining polynomial x^2 + 2
  Defn:   Generic morphism:
          From: Number Field in b with defining polynomial x^2 + 2
          To:   Number Field in a with defining polynomial x^2 + 1/2
          Defn: b -> -2*a
        then
          Call morphism:
          From: Number Field in a with defining polynomial x^2 + 1/2
          To:   Full MatrixSpace of 2 by 2 dense matrices over Number Field in b with defining polynomial x^2 + 2
sage: PowerSeriesRing(L, 'x').coerce_map_from(L)
Composite map:
  From: Number Field in b with defining polynomial x^2 + 2
  To:   Power Series Ring in x over Number Field in b with defining polynomial x^2 + 2
  Defn:   Generic morphism:
          From: Number Field in b with defining polynomial x^2 + 2
          To:   Number Field in a with defining polynomial x^2 + 1/2
          Defn: b -> -2*a
        then
          Conversion map:
          From: Number Field in a with defining polynomial x^2 + 1/2
          To:   Power Series Ring in x over Number Field in b with defining polynomial x^2 + 2

and at worst the construction itself fails (well, I'm assuming it's the same issue as above, but I didn't check in detail):

sage: L.extension(x^3+x+1, 'c')
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-28-b42564719919> in <module>()
----> 1 L.extension(x**Integer(3)+x+Integer(1), 'c')

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/rings/number_field/number_field.pyc in extension(self, poly, name, names, check, embedding)
   4173             raise TypeError, "the variable name must be specified."
   4174         from sage.rings.number_field.number_field_rel import NumberField_relative
-> 4175         return NumberField_relative(self, poly, str(name), check=check, embedding=embedding)
   4176 
   4177     def factor(self, n):

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/rings/number_field/number_field_rel.pyc in __init__(self, base, polynomial, name, latex_name, names, check, embedding)
    301 
    302         v[0] = self._gen_relative()
--> 303         v = [self(x) for x in v]
    304         self.__gens = tuple(v)
    305         self._zero_element = self(0)

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:8078)()

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.convert_map_from (sage/structure/parent.c:15826)()

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.discover_convert_map_from (sage/structure/parent.c:15991)()

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.coerce_map_from (sage/structure/parent.c:14563)()

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.discover_coerce_map_from (sage/structure/parent.c:14932)()

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.coerce_map_from (sage/structure/parent.c:14563)()

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.discover_coerce_map_from (sage/structure/parent.c:14985)()

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent_old.so in sage.structure.parent_old.Parent._coerce_map_from_ (sage/structure/parent_old.c:6658)()

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/rings/number_field/number_field_rel.pyc in _coerce_map_from_(self, R)
   1031         mor = self.base_field().coerce_map_from(R)
   1032         if mor is not None:
-> 1033             return self.coerce_map_from(self.base_field()) * mor
   1034 
   1035     def _rnfeltreltoabs(self, element, check=False):

/home/marc/co/sage/local/lib/python2.7/site-packages/sage/categories/map.so in sage.categories.map.Map.__mul__ (sage/categories/map.c:4796)()

AttributeError: 'NoneType' object has no attribute 'domain'

[mezzarobba]

Changed keywords from none to embedding

[mezzarobba]

Author: Marc Mezzarobba

[mezzarobba]

Branch: u/mmezzarobba/coerce_embeddings

[mezzarobba]

comment:3

Here's an attempt to fix the problem with a small change to the coercion discovery algorithm. It does break a few things, but I'd argue it only does so by exposing weaknesses in existing code. The other patches in the series fix (or, in one case, work around) these weaknesses.

See the commit messages for details.

What do the experts say?

[simon-king-jena]

comment:5

Replying to @mezzarobba:

Here's an attempt to fix the problem with a small change to the coercion discovery algorithm. It does break a few things, but I'd argue it only does so by exposing weaknesses in existing code. The other patches in the series fix (or, in one case, work around) these weaknesses.

See the commit messages for details.

What do the experts say?

I couldn't tell, unless you give me a pointer to the new workfow. What do I need to do, when not a patch but a brunch (aaahm, branch...) is given?

[simon-king-jena]

comment:6

Replying to @simon-king-jena:

I couldn't tell, unless you give me a pointer to the new workfow. What do I need to do, when not a patch but a brunch (aaahm, branch...) is given?

For example, when I look at the "commits" link, I see that there are three commits, and only one of them seems to refer to this ticket. So, what do I do with the other commits, review-wise? I see that there is a discussion on sage-git on the question what exactly is to be reviewed. Would I only review the last commit (since its commit message refers to this ticket)? Would I review all three commits (but then I would potentially also review dependencies that are reviewed elsewhere!)?

[mezzarobba]

comment:7

Replying to @simon-king-jena:

Replying to @simon-king-jena:

I couldn't tell, unless you give me a pointer to the new workfow. What do I need to do, when not a patch but a brunch (aaahm, branch...) is given?

If you already have a git-based installation of sage (i.e. you once did something like git clone https://github.com/sagemath/sage.git && git checkout -b origin/build_system && make build) and a remote that refers to the trac server, you just need to fetch and checkout the branch u/mmezzarobba/coerce_embeddings from trac and review the three commits on top of build_system just like you would if they were patches you just applied.

Otherwise, hmm, I guess it is easier to set up a git-based installation than to try to do the review with Mercurial... See http://sagemath.github.io/git-developer-guide/ (or ask!) in case you need more information on how to do that.

If the branch was based off an old version of sage, I guess you might also want to check that it merges cleanly, but here it is already a descendant of the last beta.

For example, when I look at the "commits" link, I see that there are three commits, and only one of them seems to refer to this ticket. So, what do I do with the other commits, review-wise? I see that there is a discussion on sage-git on the question what exactly is to be reviewed. Would I only review the last commit (since its commit message refers to this ticket)? Would I review all three commits (but then I would potentially also review dependencies that are reviewed elsewhere!)?

All three commits need review, and none of them is part of any other ticket. I am not sure if/where the ticket number should go in the commit logs (perhaps it would make more sense to put it in the message associated to the merge commit?). The last commit of the branch is also the one that fixes the issue the ticket is about, so I put it here, but I can rewrite history to add it to the two other commits if you want.

[simon-king-jena]

comment:8

Replying to @mezzarobba:

If you already have a git-based installation of sage (i.e. you once did something like git clone https://github.com/sagemath/sage.git && git checkout -b origin/build_system && make build)

I have. But at least on one of my machines I get several doctest errors with the latest git version.

and a remote that refers to the trac server,

I did store my ssh keys on trac. But "having a remote" sounds like I need to inform git about the existence of the trac server.

you just need to fetch and checkout the branch u/mmezzarobba/coerce_embeddings from trac

I hope the meaning and how-to of "fetch" and "checkout" will be clear to me from the git-developer-guide.

http://sagemath.github.io/git-developer-guide/ (or ask!) in case you need more information on how to do that.

I saw this (or an old version) before, which made me register my ssh keys, for example.

All three commits need review, and none of them is part of any other ticket. I am not sure if/where the ticket number should go in the commit logs (perhaps it would make more sense to put it in the message associated to the merge commit?). The last commit of the branch is also the one that fixes the issue the ticket is about, so I put it here, but I can rewrite history to add it to the two other commits if you want.

No idea about this. From the discussion on sage-git, it seems to me that the git branches can be totally orthogonal to what one used to do with patches. A branch may span over different tickets, and it is totally unclear from the commits themselves which of them belong to a ticket and which of them belong to a dependency. That's bad for reviewing. In the old model, one would talk about a list of patches attached to the ticket, which makes it crystal clear what ticket a patch belongs to.

[simon-king-jena]

comment:9

Replying to @mezzarobba:

Otherwise, hmm, I guess it is easier to set up a git-based installation than to try to do the review with Mercurial... See http://sagemath.github.io/git-developer-guide/ (or ask!) in case you need more information on how to do that.

It fails totally, it seems.

Namely, in my git install of sage (which I kept up to date by git pull followed by make), I tried

simon@linux-sqwp:~/SAGE/GIT/sage> ./sage -dev create-ticket
sage-run received unknown option: -dev 
usage: sage [options]
Try 'sage -h' for more information.

sage -dev create-ticket is about the first example of using the development scripts in the development guide!

[mezzarobba]

comment:10

Replying to @simon-king-jena:

It fails totally, it seems.

Sorry, I always use git directly and didn't realize that de developer guide presented the custom devel scripts first.

[mezzarobba]

comment:11

(It looks like you got most of this sorted out, based on you posts on sage-git. I'm replying anyway just in case; please feel free to ask for more detail!)

Replying to @simon-king-jena:

Replying to @mezzarobba:

If you already have a git-based installation of sage (i.e. you once did something like git clone https://github.com/sagemath/sage.git && git checkout -b origin/build_system && make build)

I have. But at least on one of my machines I get several doctest errors with the latest git version.

Yes. These are rather minor issues that are handled elsewhere.

and a remote that refers to the trac server,

I did store my ssh keys on trac. But "having a remote" sounds like I need to inform git about the existence of the trac server.

Yes:

git remote add trac ssh://git@trac.sagemath.org:2222/sage.git

(With this setup, git fetch trac will automatically download copies of all branches available on the trac server. This will likely mean hundreds of branches when everyone switches to git. They don't live in the same namespace as your local branches and don't pollute the output of most commands. But if you don't like that behaviour, you can use -t to limit the subset of branches to track.)

you just need to fetch and checkout the branch u/mmezzarobba/coerce_embeddings from trac

I hope the meaning and how-to of "fetch" and "checkout" will be clear to me from the git-developer-guide.

• fetch = synchronize your local repository's view of the branches on trac (!= your local branches) with their remote state:

git fetch trac

• checkout = get the contents of a given revision in the form of actual files in your working directory:

git checkout -b my_local_review_branch -t trac/u/mmezzarobba/coerce_embeddings

• The last command also sets up a local branch for you to work on (e.g., to prepare a reviewer patch). If you only want to have a look at some remote branch, you can omit the -b and -t options. git will issue a scary warning about detached heads. That "detached head state" is when you don't have anywhere to commit your changes if you make any.

If you want to prepare a reviewer patch, you will need to:

• Make your changes.

• Commit them to your local review branch (git commit -a to commit everything).

• (Repeat if there are several unrelated changes.)

• Once you are happy with your local changes, push them somewhere on trac:

git push trac my_local_review_branch:trac/u/SimonKing/coerce_embeddings

• Tell me about the new branch you created on trac, or just put its name in the Branch: field.

No idea about this. From the discussion on sage-git, it seems to me that the git branches can be totally orthogonal to what one used to do with patches. A branch may span over different tickets,

A git branch is just a pointer to a commit (and thus implicitly to everything that precedes it history-wise). So in some sense it every branch spans the whole history of sage, but otherwise a branch cannot really span several tickets.

(There are several other kinds of "pointers to commits" in git. Branches are pointers to commit that move up automatically when new commits are added "on" them.)

and it is totally unclear from the commits themselves which of them belong to a ticket and which of them belong to a dependency. That's bad for reviewing. In the old model, one would talk about a list of patches attached to the ticket, which makes it crystal clear what ticket a patch belongs to.

I agree, and that's why I started this thread...

[jpflori]

Commit: 0869e9a

[jpflori]

New commits:

[changeset:0869e9a]	14982: consider coercions of parents with embeddings that do not use the embedding
[changeset:cd982ae]	Make coercions that go through lazy fields more robust
[changeset:110dd48]	Cleanup after querying the coercion system with a temporary element_class in NumberField_cyclotomic

[simon-king-jena]

comment:13

Is this branch compatible with #15303?

[mezzarobba]

comment:14

Replying to @simon-king-jena:

Is this branch compatible with #15303?

The issues do look related, but otherwise I have no idea: I stopped working on it weeks before the discussion on #15303 started, and I haven't have time to read that thread yet. I hope to be able to work on this some time next week.

[simon-king-jena]

comment:15

Replying to @mezzarobba:

Replying to @simon-king-jena:

Is this branch compatible with #15303?

The issues do look related, but otherwise I have no idea: I stopped working on it weeks before the discussion on #15303 started, and I haven't have time to read that thread yet. I hope to be able to work on this some time next week.

I have tested that #15303 does not fix the bug from your ticket description. What I meant was: Are there merge conflicts?

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 0869e9a to 4cf3ca5

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

[4cf3ca5](https://github.com/sagemath/sagetrac-mirror/commit/4cf3ca5)	Fix gross bug in Map.domains(), add missing docstrings
[17832a0](https://github.com/sagemath/sagetrac-mirror/commit/17832a0)	Merge coercion transitivity fixes (#15303)
[528a035](https://github.com/sagemath/sagetrac-mirror/commit/528a035)	Merge branch 'ticket/13394' into ticket/15303, to make weak caches safer
[851cc95](https://github.com/sagemath/sagetrac-mirror/commit/851cc95)	Avoid some pointer casts in WeakValueDict callbacks
[246518f](https://github.com/sagemath/sagetrac-mirror/commit/246518f)	Use 's internals in WeakValueDictionary and do not reinvent the bucket.
[fab0ed4](https://github.com/sagemath/sagetrac-mirror/commit/fab0ed4)	Use WeakValueDict's iteration guard more consequently
[e4adaeb](https://github.com/sagemath/sagetrac-mirror/commit/e4adaeb)	Implement copy and deepcopy for WeakValueDictionary
[70a7b8a](https://github.com/sagemath/sagetrac-mirror/commit/70a7b8a)	Guard WeakValueDictionary against deletions during iteration
[c3dba98](https://github.com/sagemath/sagetrac-mirror/commit/c3dba98)	Replace weakref.WeakValueDictionary by sage.misc.weak_dict.WeakValueDictionary
[17b0236](https://github.com/sagemath/sagetrac-mirror/commit/17b0236)	Documentation for WeakValueDictionary

[mezzarobba]

comment:17

Replying to @simon-king-jena:

I have tested that #15303 does not fix the bug from your ticket description. What I meant was: Are there merge conflicts?

Sorry for the late reply. There were a few conflicts indeed. I did the merge, and the tests still pass, except that I'm getting a timeout with sage/rings/number_field/number_field.py and random segfaults with some other files.

For instance:

$ sage -t -a
...
----------------------------------------------------------------------
sage -t src/sage/rings/number_field/number_field.py  # Time out
sage -t src/sage/rings/number_field/number_field_element_quadratic.pyx  # Killed due to segmentation fault
sage -t src/sage/groups/abelian_gps/values.py  # Killed due to segmentation fault
----------------------------------------------------------------------

but then:

$ sage -t src/sage/rings/number_field/number_field_element_quadratic.pyx
Running doctests with ID 2013-11-27-23-09-47-2b207575.
Doctesting 1 file.
sage -t src/sage/rings/number_field/number_field_element_quadratic.pyx
    [420 tests, 7.97 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

$ sage -t src/sage/groups/abelian_gps/values.py
Running doctests with ID 2013-11-27-23-10-03-7a10efd7.
Doctesting 1 file.
sage -t src/sage/groups/abelian_gps/values.py
    [81 tests, 0.11 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

[mezzarobba]

comment:18

Replying to @mezzarobba:

random segfaults with some other files.

It looks like the crashes happen in __pyx_pf_4sage_10categories_3map_3Map_6domain___get__, called from sage/structure/parent.pyx:2536...

[mezzarobba]

comment:19

Replying to @mezzarobba:

Replying to @mezzarobba:

random segfaults with some other files.

It looks like the crashes happen in __pyx_pf_4sage_10categories_3map_3Map_6domain___get__, called from sage/structure/parent.pyx:2536...

I tried again with a clean build, and I am unable to reproduce the crashes. I guess it was only a compilation problem...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

7e8d4aa	Merge coercion transitivity fixes (#15303) again
858cd39	typo
12e8055	Merge branch 'ticket/14711' into ticket/15303
ee30c20	Address the "check" keyword of scheme morphisms by name, not position
d68c5df	Minor fixes, that became needed since #14711 was not merged quickly enough
c42b539	Merge branch 'master' into ticket/14711
2712c53	Merge 'trac/master' into ticket/15303
23f18f2	Merge branch 'master' into ticket/14711

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 4cf3ca5 to 7e8d4aa

[mezzarobba]

comment:22

(git-)Rebased on top of 6.1.rc0, removed dependency on #15303.

---

New commits:

0faffde	Cleanup after querying the coercion system with a temporary element_class in NumberField_cyclotomic
bd4f772	Make coercions that go through lazy fields more robust
2fcc473	14982: consider coercions of parents with embeddings that do not use the embedding

[mezzarobba]

Description changed:

--- 
+++ 
@@ -1,3 +1,4 @@
+`Parent.discover_coerce_map_from` used to always give priority to coercions that start with applying an embedding over those provided by `_coerce_map_from_`. This leads to coercion failures such as the following:

sage: K. = NumberField(x^2+1/2, embedding=CC(0,1)) @@ -8,29 +9,8 @@ AttributeError Traceback (most recent call last)

in () ----> 1 R.coerce_map_from(R.base_ring()) - -/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.coerce_map_from (sage/structure/parent.c:14807)() - -/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.coerce_map_from (sage/structure/parent.c:14563)() - -/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.discover_coerce_map_from (sage/structure/parent.c:14932)() - -/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.coerce_map_from (sage/structure/parent.c:14563)() - -/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.discover_coerce_map_from (sage/structure/parent.c:14985)() - -/home/marc/co/sage/local/lib/python2.7/site-packages/sage/structure/parent_old.so in sage.structure.parent_old.Parent._coerce_map_from_ (sage/structure/parent_old.c:6658)() - -/home/marc/co/sage/local/lib/python2.7/site-packages/sage/rings/polynomial/polynomial_ring.pyc in _coerce_map_from_(self, P) - 560 connecting = base_ring.coerce_map_from(P) - 561 if connecting is not None: ---> 562 return self.coerce_map_from(base_ring) * connecting - 563 except TypeError: - 564 pass - -/home/marc/co/sage/local/lib/python2.7/site-packages/sage/categories/map.so in sage.categories.map.Map.__mul__ (sage/categories/map.c:4796)() - +... AttributeError: 'NoneType' object has no attribute 'domain' ``` -Note that apparently P is K (not L) in the call to `_coerce_map_from_` at the end of the backtrace. +This patch series modifies the coercion discovery algorithm to fix the issue, and fixes or works around a few weaknesses in existing code exposed by the change. ``````

Changed branch from u/mmezzarobba/coerce_embeddings to u/mmezzarobba/14982-coerce_embeddings

[mezzarobba]

Changed commit from 7e8d4aa to 2fcc473

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2fcc473 to b781678

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

693dfc1	Cleanup after querying the coercion system with a temporary element_class in NumberField_cyclotomic
372553a	Make coercions that go through lazy fields more robust
b781678	14982: consider coercions of parents with embeddings that do not use the embedding

[mezzarobba]

comment:26

Replying to @sagetrac-git:

Branch pushed to git repo; I updated commit sha1. This was a forced push.

Rebased. Tests pass.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from b781678 to 873e359

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d73e507	Cleanup after querying the coercion system with a temporary element_class in NumberField_cyclotomic
8318f0c	Make coercions that go through lazy fields more robust
873e359	14982: consider coercions of parents with embeddings that do not use the embedding

[mezzarobba]

comment:30

Rebased. (Tests are still running.)

---

New commits:

d73e507	Cleanup after querying the coercion system with a temporary element_class in NumberField_cyclotomic
8318f0c	Make coercions that go through lazy fields more robust
873e359	14982: consider coercions of parents with embeddings that do not use the embedding

[jpflori]

comment:32

Needs work for what?

[mezzarobba]

comment:33

Replying to @jpflori:

Needs work for what?

There are new test failures with the rebase. I haven't had time to fix them yet...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 873e359 to 3cfa543

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3d7d823	#14982 Clean up after querying the coercion system with a temporary element_class in NumberField_cyclotomic
434c73d	#14982 Make coercions that go through lazy fields more robust
749ec1f	#14982 Consider coercions of parents with embeddings that do not use the embedding
3cfa543	#14982 Workaround for q_binomial doctest

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

b40ed6c

#14982 Robustify PolynomialRing_general._coerce_map_from_()

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 3cfa543 to b40ed6c

[mezzarobba]

comment:37

(I haven't had time to run all tests with the last commit yet, only those that failed before plus modules related to things that changed.)

[mezzarobba]

Description changed:

--- 
+++ 
@@ -13,4 +13,4 @@
 AttributeError: 'NoneType' object has no attribute 'domain'

-This patch series modifies the coercion discovery algorithm to fix the issue, and fixes or works around a few weaknesses in existing code exposed by the change. +This patch series modifies the coercion discovery algorithm to fix the issue, and fixes or works around a few weaknesses in existing code exposed by the change. See the individual commit messages for details.

[mezzarobba]

comment:39

All tests pass.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ce4e0a8	#14982 Clean up after querying the coercion system with a temporary element_class in NumberField_cyclotomic
ce34b1e	#14982 Make coercions that go through lazy fields more robust
16df502	#14982 Consider coercions of parents with embeddings that do not use the embedding
3f9b096	#14982 Workaround for q_binomial doctest
f32b52f	#14982 Robustify PolynomialRing_general._coerce_map_from_()

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from b40ed6c to f32b52f

[videlec]

comment:41

Hello,

I have a question related to number fields (in particular #18226 and #17830). I do want a direct coercion number field -> RIF based on the (future) _mpfr_ method of elements. Right now, the route is long as the discoevery algorithm tells me that I need to do number field -> RLF -> RIF (and actually there is QQbar hidden in the middle). I guess that the proposition here would solve this issue! That would be fantastic.

Vincent

[mezzarobba]

comment:42

Replying to @videlec:

I have a question related to number fields (in particular #18226 and #17830). I do want a direct coercion number field -> RIF based on the (future) _mpfr_ method of elements. Right now, the route is long as the discoevery algorithm tells me that I need to do number field -> RLF -> RIF (and actually there is QQbar hidden in the middle). I guess that the proposition here would solve this issue! That would be fantastic.

I don't know if the changes in this ticket would suffice to discover the right coercion path, but at least they make it possible to select a coercion path that doesn't use the embedding even when there is another path that uses it!

[videlec]

comment:43

Hello,

In this comment

+            # As a consequence, a value of __an_element with the wrong class
+            # is cached during the call to has_coerce_map_from. We reset the
+            # cache afterwards.

you refer to __an_element. But you changed the attribute's name to _cache_an_element.

The hack for quadratic number field is horrible... At least could we replace

diff --git a/src/sage/rings/number_field/number_field_element_quadratic.pyx b/src/sage/rings/number_field/number_field_element_quadratic.pyx
index 32e1acc..692bae1 100644
--- a/src/sage/rings/number_field/number_field_element_quadratic.pyx
+++ b/src/sage/rings/number_field/number_field_element_quadratic.pyx
@@ -210,21 +210,8 @@ cdef class NumberFieldElement_quadratic(NumberFieldElement_absolute):
             self._reduce_c_()

         # set the attribute standard embedding which is used in the method
-        # __cmp__
-        try:
-            self.standard_embedding = parent._standard_embedding
-        except AttributeError:
-            emb = parent.coerce_embedding()
-            if emb is None:
-                self.standard_embedding = True
-                try:
-                    parent._standard_embedding = True
-                except AttributeError:
-                    pass
-            else:
-                raise ValueError("A parent of NumberFieldElement_quadratic with "
-                                 "a canonical embedding should have an attribute "
-                                 "_standard_embedding (used for comparisons of elements)")
+        # __cmp__, sign, real, imag, floor, ceil, ...
+        self.standard_embedding = parent._standard_embedding

Vincent

[mezzarobba]

comment:44

Replying to @videlec:

In this comment

+            # As a consequence, a value of __an_element with the wrong class
+            # is cached during the call to has_coerce_map_from. We reset the
+            # cache afterwards.

you refer to __an_element. But you changed the attribute's name to _cache_an_element.

Thanks, fixed.

The hack for quadratic number field is horrible...

You are the one who introduced it, aren't you? ;-) So I guess you understand what it is for better than I do. In any case I just tested the change you suggested (on top of my branch + the current master) and it doesn't seem to break anything.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from f32b52f to 4b55415