Remove no_generic_basering_coercion

[pjbruin]

After #19225, the ad-hoc no_generic_basering_coercion attribute introduced in #11900 can be replaced by suitable _coerce_map_from_base_ring() methods.

CC: @tscrim

Component: coercion

Author: Peter Bruin

Branch: db76cac

Reviewer: Travis Scrimshaw

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

[pjbruin]

Branch: u/pbruin/29247-no_generic_basering_coercion

[pjbruin]

Commit: bd97ff9

[pjbruin]

comment:2

A few remarks:

• The result of _coerce_map_from_base_ring() is now modified to have weak references to the domain and codomain when registering the map as a coercion, as is done for other coercion maps
• Jordan algebras: _no_generic_basering_coercion was not used anymore since cartesian product of algebra has troubles with base_ring #19225
• MPolynomialRing_libsingular: this is a somewhat peculiar case. Since this is a Cython class, its type cannot be modified by the category framework, which results in the category-specific __init_extra__() methods not being called in Parent.__init__(). However, when inheriting from this class in Python, these methods are called, which is what caused the crash in Inheriting from MPolynomialRing_libsingular crashes sage #26958. This branch adds a common _coerce_map_from_base_ring() for all multivariate polynomial rings, which is called by __init_extra__() for Python classes and (if needed) by _coerce_map_from_() for the Cython class MPolynomialRing_libsingular. Maybe Parent.__init__() should eventually be adapted to call the category-specific __init_extra__() methods also for Cython classes, but this should be done on a separate ticket.
• Skew polynomial rings: _no_generic_basering_coercion was not used (anymore?) because these rings are not in the category of algebras.

[tscrim]

comment:3

Replying to @pjbruin:

A few remarks:

• The result of _coerce_map_from_base_ring() is now modified to have weak references to the domain and codomain when registering the map as a coercion, as is done for other coercion maps

This is done automatically by the coercion system when constructed via _coerce_map_from_, correct?

• Maybe Parent.__init__() should eventually be adapted to call the category-specific __init_extra__() methods also for Cython classes, but this should be done on a separate ticket.

It definitely should be a separate ticket that might need some design discussions.

• Skew polynomial rings: _no_generic_basering_coercion was not used (anymore?) because these rings are not in the category of algebras.

I think the proper thing to do, which should be done on this ticket, is to promote it to the category of unital algebras and use the new mechanism here.

[sagetrac-git]

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

128194e	Trac 29247: put skew polynomial rings into the category of algebras
500219a	Trac 29247: some cleaning up

[sagetrac-git]

Changed commit from bd97ff9 to 500219a

[pjbruin]

comment:5

Replying to @tscrim:

Replying to @pjbruin:

A few remarks:

• The result of _coerce_map_from_base_ring() is now modified to have weak references to the domain and codomain when registering the map as a coercion, as is done for other coercion maps

This is done automatically by the coercion system when constructed via _coerce_map_from_, correct?

Yes.

• Skew polynomial rings: _no_generic_basering_coercion was not used (anymore?) because these rings are not in the category of algebras.

I think the proper thing to do, which should be done on this ticket, is to promote it to the category of unital algebras and use the new mechanism here.

At first I thought this would be the wrong thing to do because the definition of algebras that I am used to implies that if A is an algebra over a ring R, then left multiplication and right multiplication by an element of R have the same effect. However, it seems this condition is not imposed by Sage, so giving an R-algebra is equivalent to giving a (unital, associative) ring A and a ring homomorphism from R to A.

[tscrim]

comment:6

Replying to @pjbruin:

Replying to @tscrim:

Replying to @pjbruin:

• Skew polynomial rings: _no_generic_basering_coercion was not used (anymore?) because these rings are not in the category of algebras.

I think the proper thing to do, which should be done on this ticket, is to promote it to the category of unital algebras and use the new mechanism here.

At first I thought this would be the wrong thing to do because the definition of algebras that I am used to implies that if A is an algebra over a ring R, then left multiplication and right multiplication by an element of R have the same effect. However, it seems this condition is not imposed by Sage, so giving an R-algebra is equivalent to giving a (unital, associative) ring A and a ring homomorphism from R to A.

My understanding is that for (magmatic) algebras, a left/right R-algebra has a left/right R-module structure, but nothing says that there has to be both or that these structures are the same. For unital algebras, the fact that 1 commutes is equivalent to saying that we have the ring homomorphism from R to A that you mentioned.

LGTM. Thanks.

[tscrim]

Reviewer: Travis Scrimshaw

[vbraun]

comment:7

Merge conflict

[pjbruin]

comment:8

Replying to @vbraun:

Merge conflict

Any info as to what is the conflicting ticket? I just tried

git pull https://github.com/vbraun/sage develop

and got no conflicts.

[sagetrac-git]

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

db76cac

Trac 29247: change doctest to prevent merge conflict with #28882

[sagetrac-git]

Changed commit from 500219a to db76cac

[pjbruin]

comment:10

Replying to @pjbruin:

Replying to @vbraun:

Merge conflict

Any info as to what is the conflicting ticket? I just tried

git pull https://github.com/vbraun/sage develop

and got no conflicts.

My mistake, I did this on the wrong branch. The conflict was with #28882.

[vbraun]

Changed branch from u/pbruin/29247-no_generic_basering_coercion to db76cac

[mezzarobba]

Changed commit from db76cac to none

[mezzarobba]

comment:12

It seems that this ticket does more than just deprecating _no_generic_basering_coercion -- it can break code that still uses it. For example, with ore_algebra, I get:

sage: Dops_x, x, Dx = DifferentialOperators()
/home/marc/docs/code/sage/ore_algebra/ore_algebra/src/ore_algebra/ore_algebra.py:1019: DeprecationWarning: the attribute _no_generic_basering_coercion is deprecated, implement _coerce_map_from_base_ring() instead
See http://trac.sagemath.org/19225 for details.
  super(self.__class__, self).__init__(base_ring)
sage: x + Dx
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-2-71f2b2c8bc9b> in <module>()
----> 1 x + Dx

/home/marc/co/sage/local/lib/python3.7/site-packages/sage/structure/element.pyx in sage.structure.element.Element.__add__ (build/cythonized/sage/structure/element.c:10838)()
   1232         # Left and right are Sage elements => use coercion model
   1233         if BOTH_ARE_ELEMENT(cl):
-> 1234             return coercion_model.bin_op(left, right, add)
   1235 
   1236         cdef long value

/home/marc/co/sage/local/lib/python3.7/site-packages/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel.bin_op (build/cythonized/sage/structure/coerce.c:10117)()
   1205         # Now coerce to a common parent and do the operation there
   1206         try:
-> 1207             xy = self.canonical_coercion(x, y)
   1208         except TypeError:
   1209             self._record_exception()

/home/marc/co/sage/local/lib/python3.7/site-packages/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel.canonical_coercion (build/cythonized/sage/structure/coerce.c:11700)()
   1324                 y_elt = y
   1325             if x_elt is None:
-> 1326                 raise RuntimeError("BUG in map, returned None %s %s %s" % (x, type(x_map), x_map))
   1327             elif y_elt is None:
   1328                 raise RuntimeError("BUG in map, returned None %s %s %s" % (y, type(y_map), y_map))

RuntimeError: BUG in map, returned None x <class 'sage.categories.morphism.SetMorphism'> (map internal to coercion system -- copy before use)
Generic morphism:
  From: Univariate Polynomial Ring in x over Rational Field
  To:   Univariate Ore algebra in Dx over Univariate Polynomial Ring in x over Rational Field

Defining _coerce_map_from_base_ring() fixes the issue.

I personally don't really care (external users of this kind of things probably have to adapt their code with every release anyway), but if it is easy to preserve compatibility with older versions, it may be a good think nevertheless.

[pjbruin]

comment:13

Replying to @mezzarobba:

It seems that this ticket does more than just deprecating _no_generic_basering_coercion -- it can break code that still uses it.

You are right, I accidentally deleted the return statement while adding the deprecation. This should be fixed by #29303.

[mwageringel]

comment:14

Another problem that seems to be caused by this ticket:

sage: R.<x,y,z> = QQ[]
sage: from sage.libs.singular.function_factory import ff
sage: W = ff.ring(ff.ringlist(R), ring=R)
sage: C = sage.rings.polynomial.plural.new_CRing(W, R.base_ring())
sage: C.one()   # should be 1
0

Presumably, this happens because the coercion from Python ints is a composite map via QQ now

sage: C._internal_coerce_map_from(int)
(map internal to coercion system -- copy before use)
Composite map:
  From: Set of Python objects of class 'int'
  To:   Multivariate Polynomial Ring in x, y, z over Rational Field
sage: C._internal_coerce_map_from(int)[0]
(map internal to coercion system -- copy before use)
Native morphism:
  From: Set of Python objects of class 'int'
  To:   Rational Field

but the coercion from QQ to C calls one() again, resulting in a cycle. In 9.1.beta6, this was not a composite map:

sage: C._internal_coerce_map_from(int)
(map internal to coercion system -- copy before use)
Coercion map:
  From: Set of Python objects of class 'int'
  To:   Multivariate Polynomial Ring in x, y, z over Rational Field

Do you have any ideas how to fix this? This came up in #25993.

Another possible problem seems to be the following, but I am not sure whether letterplace algebras are missing from_base_ring or whether its existence should not be assumed:

sage: F.<x,y,z> = FreeAlgebra(QQ, implementation='letterplace')
sage: F._coerce_map_from_base_ring()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-069acb379d83> in <module>()
----> 1 F._coerce_map_from_base_ring()

.../local/lib/python3.7/site-packages/sage/categories/unital_algebras.py in _coerce_map_from_base_ring(self)
    212             # be used unconditionally.
    213             generic_from_base_ring = self.category().parent_class.from_base_ring
--> 214             if type(self).from_base_ring != generic_from_base_ring:
    215                 # Custom from_base_ring()
    216                 use_from_base_ring = True

AttributeError: type object 'sage.algebras.letterplace.free_algebra_letterplace' has no attribute 'from_base_ring'

[tscrim]

comment:15

I think the solution would be to define:

def _coerce_map_from_base_ring(self):
    return self._generic_coerce_map(self.base_ring())

which then redirects the QQ call to the _element_constructor_, which should handle it correctly. I will need to test it.

What was happening before is in _coerce_map_from_, it ends with this:

        elif base_ring.has_coerce_map_from(other):
            return True

which ultimately just means the _element_constructor_ handles it. Thus it doesn't know about any coercion chains. However, I believe (but not 100% sure) when explicitly setting the coercion map to QQ, the coercion framework looks at coercions into QQ and then chains them together. That is at least my best guess about what is happening any why this is different. However, I don't have time tonight to verify this by looking at the coercion cache at each step and do a more detailed analysis. I will try to do this tomorrow or the next day, but I don't think I can promise it.

[pjbruin]

comment:16

Replying to @mwageringel:

Another problem that seems to be caused by this ticket:

sage: R.<x,y,z> = QQ[]
sage: from sage.libs.singular.function_factory import ff
sage: W = ff.ring(ff.ringlist(R), ring=R)
sage: C = sage.rings.polynomial.plural.new_CRing(W, R.base_ring())
sage: C.one()   # should be 1
0

I think the best way to fix this problem is to initialise _one_element and _one_element_poly in new_CRing in the same way as it is done in MPolynomialRing_libsingular.__init__():

--- a/src/sage/rings/polynomial/plural.pyx
+++ b/src/sage/rings/polynomial/plural.pyx
@@ -126,7 +126,7 @@ from sage.rings.finite_rings.finite_field_prime_modn import FiniteField_prime_mo
 from sage.rings.integer cimport Integer
 from sage.rings.integer_ring import is_IntegerRing

-from sage.rings.polynomial.multi_polynomial_libsingular cimport MPolynomialRing_libsingular
+from sage.rings.polynomial.multi_polynomial_libsingular cimport MPolynomialRing_libsingular, MPolynomial_libsingular, new_MP
 from sage.rings.polynomial.multi_polynomial_ideal import NCPolynomialIdeal

 from sage.rings.polynomial.polydict import ETuple
@@ -2860,13 +2860,16 @@ cpdef MPolynomialRing_libsingular new_CRing(RingWrap rw, base_ring):
         sage: curcnt = ring_refcount_dict[currRing_wrapper()]
         sage: newR = new_CRing(W, H.base_ring())
         sage: ring_refcount_dict[currRing_wrapper()] - curcnt
-        1
+        2
     """
     assert( rw.is_commutative() )

     cdef MPolynomialRing_libsingular self = <MPolynomialRing_libsingular>MPolynomialRing_libsingular.__new__(MPolynomialRing_libsingular)

     self._ring = rw._ring
+    cdef MPolynomial_libsingular one = new_MP(self, p_ISet(1, self._ring))
+    self._one_element = one
+    self._one_element_poly = one._poly

     wrapped_ring = wrap_ring(self._ring)
     sage.libs.singular.ring.ring_refcount_dict[wrapped_ring] += 1

I compared this fix (which keeps the composite coercion map int -> QQ -> C) and the one Travis suggested in comment:15 (which reverts it to the way it was before); somewhat surprisingly, the composite map is about 30% faster.

[pjbruin]

comment:17

Replying to @mwageringel:

Another possible problem seems to be the following, but I am not sure whether letterplace algebras are missing from_base_ring or whether its existence should not be assumed:

sage: F.<x,y,z> = FreeAlgebra(QQ, implementation='letterplace')
sage: F._coerce_map_from_base_ring()
...
AttributeError: type object 'sage.algebras.letterplace.free_algebra_letterplace' has no attribute 'from_base_ring'

Python classes inherit a from_base_ring() method from the category of unital algebras when they are placed in that category, but Cython classes do not, because the category framework cannot change its type (see also comment:2). I suggest this:

--- a/src/sage/categories/unital_algebras.py
+++ b/src/sage/categories/unital_algebras.py
@@ -212,7 +212,8 @@ class UnitalAlgebras(CategoryWithAxiom_over_base_ring):
             # If there is a specialised from_base_ring(), then it should
             # be used unconditionally.
             generic_from_base_ring = self.category().parent_class.from_base_ring
-            if type(self).from_base_ring != generic_from_base_ring:
+            f = getattr(type(self), 'from_base_ring', None)
+            if f is not None and f != generic_from_base_ring:
                 # Custom from_base_ring()
                 use_from_base_ring = True
             if isinstance(generic_from_base_ring, lazy_attribute):

[tscrim]

comment:18

The composite map is probably faster because QQ has specialized routines for converting int because of mpq and then creating the polynomial with elements already in QQ is likely very optimized as well. So I would go with Peter's solution in comment:16.

[pjbruin]

comment:19

See #29311 and #29312 (the latter with a better solution than the one I suggested in comment:17).