has_custom_conversion check in UnitalAlgebras.ParentMethods is broken

[jdemeyer]

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.

CC: @nthiery

Component: categories

Keywords: sagedays@icerm python3

Author: Jeroen Demeyer

Branch/Commit: 003b22a

Reviewer: Travis Scrimshaw, Nicolas M. Thiéry

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

[jdemeyer]

Branch: u/jdemeyer/has_custom_conversion_check_in_unitalalgebras_parentmethods_is_broken

[jdemeyer]

Commit: 195e94a

[jdemeyer]

New commits:

195e94a

Fix has_custom_conversion check in UnitalAlgebras.ParentMethods

[jdemeyer]

comment:4

@nthiery: it would be great to have some advice on how to proceed...

[embray]

comment:5

So use six.get_unbound_function on both sides. The function name is a bit of a mouthful, but it's very accurate and unambiguous as to what it does...

Another way to spell it is the_func = getattr(the_method, '__func__', the_method) but it's a little less obvious what the intent is of that.

[jdemeyer]

comment:6

I meant advice on what to do with the lazy attribute situation. The comments do not agree with the code. So either we fix the comments to say what the code really does or we change the code to do what the comments say. Unfortunately, the latter breaks stuff (see ticket description).

[jdemeyer]

comment:7

Replying to @embray:

So use six.get_unbound_function on both sides.

Not both sides, because one side is a bound method and the other is an unbound method.

[jdemeyer]

comment:8

Nicolas: I would love to hear your opinion on this ticket.

[jdemeyer]

comment:9

Given that this seems difficult to fix properly, I'll probably go for cleaning it up without making any functional changes and properly documenting what happens.

[embray]

comment:10

This is already "fixed" in #24955 but only superficially.

[sagetrac-git]

Changed commit from 195e94a to 003b22a

[sagetrac-git]

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

003b22a

Refactor UnitalAlgebras.ParentMethods.__init_extra__

[jdemeyer]

comment:15

Travis told me in person that he is willing to set this to positive review later this month, after discussing it with Nicolas.

[jdemeyer]

Changed keywords from none to sagedays@icerm

[embray]

Changed keywords from sagedays@icerm to sagedays@icerm python3

[embray]

comment:18

Adding python3 keyword since this helps fix a number of bugs there as well.

[nthiery]

comment:19

Hi Jeroen!

I have just been through the new version. It is now much much clearer what the current logic does; thanks a lot! The logic is still crappy and way too complicated. but it's indeed very reasonable to split the work in two stages (in this ticket: fixing the Python 3 issue + clarifying the current logic; in a later ticket:fix/improve the logic).

Positive review on my side.

Just one question: while rewriting the code, did you do some additional manual tests to check how things worked? Or were the current doctests sufficient? In the former case, in case you still have your manual tests under hand, could you add them to the doctests?

Thanks again,

[nthiery]

comment:20

Travis is next to me and agrees with the above comment. I let you set the positive review on our behalf after the question above.

[nthiery]

Reviewer: Travis Scrimshaw, Nicolas M. Thiéry

[jdemeyer]

comment:22

Replying to @nthiery:

Just one question: while rewriting the code, did you do some additional manual tests to check how things worked?

I don't think so.

Or were the current doctests sufficient?

I recall from my work in Cernay on this ticket that the code was quite brittle: any small change in functionality would induce doctest failures somewhere. Maybe there are not many explicit tests, but the code is used in many places and therefore well tested implicitly.

[vbraun]

Changed branch from u/jdemeyer/has_custom_conversion_check_in_unitalalgebras_parentmethods_is_broken to 003b22a