Opened 8 months ago

Closed 8 months ago

Last modified 7 months ago

#29247 closed enhancement (fixed)

Remove no_generic_basering_coercion

Reported by: pbruin Owned by:
Priority: minor Milestone: sage-9.1
Component: coercion Keywords:
Cc: tscrim Merged in:
Authors: Peter Bruin Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: db76cac (Commits) Commit:
Dependencies: Stopgaps:

Description

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

Change History (19)

comment:1 Changed 8 months ago by pbruin

  • Branch set to u/pbruin/29247-no_generic_basering_coercion
  • Commit set to bd97ff9f189490d9e04ce8d22d6f564b9884ea16
  • Status changed from new to needs_review

comment:2 follow-up: Changed 8 months ago by pbruin

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

comment:3 in reply to: ↑ 2 ; follow-up: Changed 8 months ago by tscrim

Replying to pbruin:

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.

comment:4 Changed 8 months ago by git

  • Commit changed from bd97ff9f189490d9e04ce8d22d6f564b9884ea16 to 500219a7302262b2ec53f22eccbdbca9b6f5ae8d

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

128194eTrac 29247: put skew polynomial rings into the category of algebras
500219aTrac 29247: some cleaning up

comment:5 in reply to: ↑ 3 ; follow-up: Changed 8 months ago by pbruin

Replying to tscrim:

Replying to pbruin:

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.

comment:6 in reply to: ↑ 5 Changed 8 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Replying to pbruin:

Replying to tscrim:

Replying to pbruin:

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

comment:7 follow-up: Changed 8 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:8 in reply to: ↑ 7 ; follow-up: Changed 8 months ago by pbruin

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.

comment:9 Changed 8 months ago by git

  • Commit changed from 500219a7302262b2ec53f22eccbdbca9b6f5ae8d to db76cac22774408fe7a15a03b78ccf88bec8c72f

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

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

comment:10 in reply to: ↑ 8 Changed 8 months ago by pbruin

  • Status changed from needs_work to positive_review

Replying to pbruin:

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.

comment:11 Changed 8 months ago by vbraun

  • Branch changed from u/pbruin/29247-no_generic_basering_coercion to db76cac22774408fe7a15a03b78ccf88bec8c72f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 follow-up: Changed 8 months ago by mmezzarobba

  • Commit db76cac22774408fe7a15a03b78ccf88bec8c72f deleted

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.

comment:13 in reply to: ↑ 12 Changed 8 months ago by pbruin

Replying to mmezzarobba:

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.

comment:14 follow-ups: Changed 8 months ago by gh-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

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'

comment:15 Changed 8 months ago by tscrim

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.

comment:16 in reply to: ↑ 14 Changed 8 months ago by pbruin

Replying to gh-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__():

  • src/sage/rings/polynomial/plural.pyx

    a b from sage.rings.finite_rings.finite_field_prime_modn import FiniteField_prime_mo 
    126126from sage.rings.integer cimport Integer
    127127from sage.rings.integer_ring import is_IntegerRing
    128128
    129 from sage.rings.polynomial.multi_polynomial_libsingular cimport MPolynomialRing_libsingular
     129from sage.rings.polynomial.multi_polynomial_libsingular cimport MPolynomialRing_libsingular, MPolynomial_libsingular, new_MP
    130130from sage.rings.polynomial.multi_polynomial_ideal import NCPolynomialIdeal
    131131
    132132from sage.rings.polynomial.polydict import ETuple
    cpdef MPolynomialRing_libsingular new_CRing(RingWrap rw, base_ring): 
    28602860        sage: curcnt = ring_refcount_dict[currRing_wrapper()]
    28612861        sage: newR = new_CRing(W, H.base_ring())
    28622862        sage: ring_refcount_dict[currRing_wrapper()] - curcnt
    2863         1
     2863        2
    28642864    """
    28652865    assert( rw.is_commutative() )
    28662866
    28672867    cdef MPolynomialRing_libsingular self = <MPolynomialRing_libsingular>MPolynomialRing_libsingular.__new__(MPolynomialRing_libsingular)
    28682868
    28692869    self._ring = rw._ring
     2870    cdef MPolynomial_libsingular one = new_MP(self, p_ISet(1, self._ring))
     2871    self._one_element = one
     2872    self._one_element_poly = one._poly
    28702873
    28712874    wrapped_ring = wrap_ring(self._ring)
    28722875    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.

comment:17 in reply to: ↑ 14 Changed 8 months ago by pbruin

Replying to gh-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:

  • src/sage/categories/unital_algebras.py

    a b class UnitalAlgebras(CategoryWithAxiom_over_base_ring): 
    212212            # If there is a specialised from_base_ring(), then it should
    213213            # be used unconditionally.
    214214            generic_from_base_ring = self.category().parent_class.from_base_ring
    215             if type(self).from_base_ring != generic_from_base_ring:
     215            f = getattr(type(self), 'from_base_ring', None)
     216            if f is not None and f != generic_from_base_ring:
    216217                # Custom from_base_ring()
    217218                use_from_base_ring = True
    218219            if isinstance(generic_from_base_ring, lazy_attribute):

comment:18 Changed 8 months ago by tscrim

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.

comment:19 Changed 7 months ago by pbruin

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

Note: See TracTickets for help on using tickets.