Trac #25181: has_custom_conversion check in UnitalAlgebras.ParentMeth…

…ods is broken

The code in `UnitalAlgebras.ParentMethods.__init_extra__` to determine a
coercion from the base ring to the algebra is quite obscure (and
probably buggy). Moreover, it doesn't work on Python 3.

Since properly cleaning this up didn't work, I instead refactor the
code, adding plenty of comments to explain what is happening.

This is a pure refactoring: no functionality is changed.

---------------------------------------

Original ticket description kept for reference:

This code
{{{
            try:
                has_custom_conversion =
self.category().parent_class.from_base_ring.__func__ is not
self.from_base_ring.__func__
            except AttributeError:
                # Sometimes from_base_ring is a lazy attribute
                has_custom_conversion = True
}}}
is trying to determine whether `from_base_ring` comes from the category
or not. However, in the case of a lazy attribute it wrongly sets
`has_custom_conversion = True` without checking equality, even if the
lazy attribute comes from the category.

Now there are two ways to solve this:

1. Keep (and document) the current behaviour, namely that the
`has_custom_conversion = True` branch is always taken if
`from_base_ring` is a lazy attribute.

2. Fix the bug and set `has_custom_conversion = False` if
`from_base_ring` is a non-custom lazy attribute. Unfortunately, this
leads to further breakage:
{{{
sage -t src/sage/categories/with_realizations.py
**********************************************************************
File "src/sage/categories/with_realizations.py", line 71, in
sage.categories.with_realizations.WithRealizations
Failed example:
    A = Sets().WithRealizations().example(); A
Exception raised:
    Traceback (most recent call last):
      File "/usr/local/src/sage-config/local/lib/python2.7/site-
packages/sage/doctest/forker.py", line 551, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-
packages/sage/doctest/forker.py", line 961, in compile_and_execute
        exec(compiled, globs)
      File "<doctest
sage.categories.with_realizations.WithRealizations[1]>", line 1, in
<module>
        A = Sets().WithRealizations().example(); A
      File "/usr/local/src/sage-config/local/lib/python2.7/site-
packages/sage/categories/sets_cat.py", line 2526, in example
        return SubsetAlgebra(base_ring, set)
      File "sage/misc/classcall_metaclass.pyx", line 330, in
sage.misc.classcall_metaclass.ClasscallMetaclass.__call__
(build/cythonized/sage/misc/classcall_metaclass.c:1647)
        return cls.classcall(cls, *args, **kwds)
      File "sage/misc/cachefunc.pyx", line 1059, in
sage.misc.cachefunc.CachedFunction.__call__
(build/cythonized/sage/misc/cachefunc.c:6269)
        w = self.f(*args, **kwds)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-
packages/sage/structure/unique_representation.py", line 1021, in
__classcall__
        instance = typecall(cls, *args, **options)
      File "sage/misc/classcall_metaclass.pyx", line 497, in
sage.misc.classcall_metaclass.typecall
(build/cythonized/sage/misc/classcall_metaclass.c:2097)
        return (<PyTypeObject*>type).tp_call(cls, args, kwds)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-
packages/sage/categories/examples/with_realizations.py", line 186, in
__init__
        In_to_F   .register_as_coercion()
      File "sage/categories/morphism.pyx", line 276, in
sage.categories.morphism.Morphism.register_as_coercion
(build/cythonized/sage/categories/morphism.c:4666)
        self._codomain.register_coercion(self)
      File "sage/structure/parent.pyx", line 1626, in
sage.structure.parent.Parent.register_coercion
(build/cythonized/sage/structure/parent.c:15031)
        assert not (self._coercions_used and D in
self._coerce_from_hash), "coercion from {} to {} already registered or
discovered".format(D, self)
    AssertionError: coercion from The subset algebra of {1, 2, 3} over
Rational Field in the In basis to The subset algebra of {1, 2, 3} over
Rational Field in the Fundamental basis already registered or discovered
**********************************************************************
}}}

Regardless of this bug, the code to do this check can be improved. The
reason why the `__func__` access is needed in the first place is because
we are trying to compare an ''unbound'' method with a ''bound'' method.
If we have unbound methods on both sides of the equality, it can be
simplified and fixed.

URL: https://trac.sagemath.org/25181
Reported by: jdemeyer
Ticket author(s): Jeroen Demeyer
Reviewer(s): Travis Scrimshaw, Nicolas M. Thiéry

src/sage/categories/unital_algebras.py

@@ -102,53 +102,63 @@ def __init_extra__(self):
                 # We will not use any generic stuff, since a (presumably) better conversion
                 # has already been registered.
                 return
-            mor = None
+
             # This could be a morphism of Algebras(self.base_ring()); however, e.g., QQ is not in Algebras(QQ)
             H = Hom(base_ring, self, Rings()) # TODO: non associative ring!
 
-            # Idea: There is a generic method "from_base_ring", that just does multiplication with
-            # the multiplicative unit. However, the unit is constructed repeatedly, which is slow.
-            # Hence, if the unit is available *now*, then we store it.
+            # We need to register a coercion from the base ring to self.
             #
-            # However, if there is a specialised from_base_ring method, then it should be used!
-            try:
-                has_custom_conversion = self.category().parent_class.from_base_ring.__func__ is not self.from_base_ring.__func__
-            except AttributeError:
-                # Sometimes from_base_ring is a lazy attribute
-                has_custom_conversion = True
-            if has_custom_conversion:
-                mor = SetMorphism(function = self.from_base_ring, parent = H)
+            # There is a generic method from_base_ring(), that just does
+            # multiplication with the multiplicative unit. However, the
+            # unit is constructed repeatedly, which is slow.
+            # So, if the unit is available *now*, then we can create a
+            # faster coercion map.
+            #
+            # This only applies for the generic from_base_ring() method.
+            # 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:
+                # Custom from_base_ring()
+                use_from_base_ring = True
+            if isinstance(generic_from_base_ring, lazy_attribute):
+                # If the category implements from_base_ring() as lazy
+                # attribute, then we always use it.
+                # This is for backwards compatibility, see Trac #25181
+                use_from_base_ring = True
+            else:
                 try:
-                    self.register_coercion(mor)
-                except AssertionError:
-                    pass
-                return
+                    one = self.one()
+                    use_from_base_ring = False
+                except (NotImplementedError, AttributeError, TypeError):
+                    # The unit is not available, yet. But there are cases
+                    # in which it will be available later. So, we use
+                    # the generic from_base_ring() after all.
+                    use_from_base_ring = True
 
-            try:
-                one = self.one()
-            except (NotImplementedError, AttributeError, TypeError):
-                # The unit is not available, yet. But there are cases
-                # in which it will be available later. Hence:
-                mor = SetMorphism(function = self.from_base_ring, parent = H)
-            # try sanity of one._lmul_
-            if mor is None:
+            mor = None
+            if use_from_base_ring:
+                mor = SetMorphism(function=self.from_base_ring, parent=H)
+            else:
+                # We have the multiplicative unit, so implement the
+                # coercion from the base ring as multiplying with that.
+                #
+                # But first we check that it actually works. If not,
+                # then the generic implementation of from_base_ring()
+                # would fail as well so we don't use it.
                 try:
-                    if one._lmul_(base_ring.an_element()) is None:
-                        # There are cases in which lmul returns None, believe it or not.
+                    if one._lmul_(base_ring.an_element()) is not None:
+                        # There are cases in which lmul returns None,
+                        # which means that it's not implemented.
                         # One example: Hecke algebras.
-                        # In that case, the generic implementation of from_base_ring would
-                        # fail as well. Hence, unless it is overruled, we will not use it.
-                        #mor = SetMorphism(function = self.from_base_ring, parent = H)
-                        return
+                        mor = SetMorphism(function=one._lmul_, parent=H)
                 except (NotImplementedError, AttributeError, TypeError):
-                    # it is possible that an_element or lmul are not implemented.
-                    return
-                    #mor = SetMorphism(function = self.from_base_ring, parent = H)
-                mor = SetMorphism(function = one._lmul_, parent = H)
-            try:
-                self.register_coercion(mor)
-            except AssertionError:
-                pass
+                    pass
+            if mor is not None:
+                try:
+                    self.register_coercion(mor)
+                except AssertionError:
+                    pass
 
     class WithBasis(CategoryWithAxiom_over_base_ring):