Opened 4 years ago

Closed 4 years ago

#25181 closed defect (fixed)

has_custom_conversion check in UnitalAlgebras.ParentMethods is broken

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.4
Component: categories Keywords: sagedays@icerm python3
Cc: nthiery Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw, Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: 003b22a (Commits, GitHub, GitLab) Commit: 003b22af7bc99de29838c6c182fb4a0247f2224a
Dependencies: Stopgaps:

Status badges

Description (last modified by 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.
  1. 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.

Change History (23)

comment:1 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/has_custom_conversion_check_in_unitalalgebras_parentmethods_is_broken

comment:2 Changed 4 years ago by jdemeyer

  • Commit set to 195e94a533efb4349da0c0eff7128160948a1a34
  • Description modified (diff)

New commits:

195e94aFix has_custom_conversion check in UnitalAlgebras.ParentMethods

comment:3 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 4 years ago by jdemeyer

  • Description modified (diff)

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

comment:5 follow-up: Changed 4 years ago by embray

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.

comment:6 Changed 4 years ago by jdemeyer

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

comment:7 in reply to: ↑ 5 Changed 4 years ago by jdemeyer

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.

comment:8 Changed 4 years ago by jdemeyer

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

comment:9 Changed 4 years ago by jdemeyer

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.

comment:10 Changed 4 years ago by embray

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

comment:11 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 4 years ago by git

  • Commit changed from 195e94a533efb4349da0c0eff7128160948a1a34 to 003b22af7bc99de29838c6c182fb4a0247f2224a

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

003b22aRefactor UnitalAlgebras.ParentMethods.__init_extra__

comment:14 Changed 4 years ago by jdemeyer

  • Status changed from new to needs_review

comment:15 Changed 4 years ago by jdemeyer

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

comment:16 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:17 Changed 4 years ago by jdemeyer

  • Keywords sagedays@icerm added

comment:18 Changed 4 years ago by embray

  • Keywords python3 added
  • Milestone changed from sage-8.2 to sage-8.4

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

comment:19 follow-up: Changed 4 years ago by nthiery

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,

comment:20 Changed 4 years ago by nthiery

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.

comment:21 Changed 4 years ago by nthiery

  • Reviewers set to Travis Scrimshaw, Nicolas M. Thiéry

comment:22 in reply to: ↑ 19 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to positive_review

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.

comment:23 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/has_custom_conversion_check_in_unitalalgebras_parentmethods_is_broken to 003b22af7bc99de29838c6c182fb4a0247f2224a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.