#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, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Change History (19)
comment:1 Changed 2 years ago by
- 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: ↓ 3 Changed 2 years ago by
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 5 Changed 2 years ago by
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 2 years ago by
- Commit changed from bd97ff9f189490d9e04ce8d22d6f564b9884ea16 to 500219a7302262b2ec53f22eccbdbca9b6f5ae8d
comment:5 in reply to: ↑ 3 ; follow-up: ↓ 6 Changed 2 years ago by
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 mapsThis 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 2 years ago by
- 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: ↓ 8 Changed 2 years ago by
- Status changed from positive_review to needs_work
Merge conflict
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 2 years ago by
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 2 years ago by
- Commit changed from 500219a7302262b2ec53f22eccbdbca9b6f5ae8d to db76cac22774408fe7a15a03b78ccf88bec8c72f
Branch pushed to git repo; I updated commit sha1. New commits:
db76cac | Trac 29247: change doctest to prevent merge conflict with #28882
|
comment:10 in reply to: ↑ 8 Changed 2 years ago by
- Status changed from needs_work to positive_review
comment:11 Changed 2 years ago by
- 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: ↓ 13 Changed 2 years ago by
- 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 2 years ago by
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: ↓ 16 ↓ 17 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
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 126 126 from sage.rings.integer cimport Integer 127 127 from sage.rings.integer_ring import is_IntegerRing 128 128 129 from sage.rings.polynomial.multi_polynomial_libsingular cimport MPolynomialRing_libsingular 129 from sage.rings.polynomial.multi_polynomial_libsingular cimport MPolynomialRing_libsingular, MPolynomial_libsingular, new_MP 130 130 from sage.rings.polynomial.multi_polynomial_ideal import NCPolynomialIdeal 131 131 132 132 from sage.rings.polynomial.polydict import ETuple … … cpdef MPolynomialRing_libsingular new_CRing(RingWrap rw, base_ring): 2860 2860 sage: curcnt = ring_refcount_dict[currRing_wrapper()] 2861 2861 sage: newR = new_CRing(W, H.base_ring()) 2862 2862 sage: ring_refcount_dict[currRing_wrapper()] - curcnt 2863 12863 2 2864 2864 """ 2865 2865 assert( rw.is_commutative() ) 2866 2866 2867 2867 cdef MPolynomialRing_libsingular self = <MPolynomialRing_libsingular>MPolynomialRing_libsingular.__new__(MPolynomialRing_libsingular) 2868 2868 2869 2869 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 2870 2873 2871 2874 wrapped_ring = wrap_ring(self._ring) 2872 2875 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 2 years ago by
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): 212 212 # If there is a specialised from_base_ring(), then it should 213 213 # be used unconditionally. 214 214 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: 216 217 # Custom from_base_ring() 217 218 use_from_base_ring = True 218 219 if isinstance(generic_from_base_ring, lazy_attribute):
comment:18 Changed 2 years ago by
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 2 years ago by
See #29311 and #29312 (the latter with a better solution than the one I suggested in comment:17).
A few remarks:
_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_no_generic_basering_coercion
was not used anymore since #19225MPolynomialRing_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 inParent.__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 classMPolynomialRing_libsingular
. MaybeParent.__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._no_generic_basering_coercion
was not used (anymore?) because these rings are not in the category of algebras.