Implement the __init_extra__ protocol of categories for Cython classes.

[nthiery]

Component: categories

Author: Simon King

Branch/Commit: u/mkoeppe/implement_the___init_extra___protocol_of_categories_for_cython_classes_ @ 0ec1c06

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

[simon-king-jena]

comment:1

It would probably be rather easy:

Currently, we only look into the mro of the class; if the class is a Python class, then it is a subclass of the category's parent class (since the quest for __init_extra__ takes place after category initialisation). But in the case of Cython class, we simply need to look explicitly into both the class' mro and the category's parent class' mro.

It would detect a lot of Cython classes in which the call to Parent.__init__ happens too early for __init_extra__ to work... But these would be exactly the classes that currently do not allow Python subclasses.

[nthiery]

comment:2

Replying to @simon-king-jena:

It would probably be rather easy:

Currently, we only look into the mro of the class; if the class is a Python class, then it is a subclass of the category's parent class (since the quest for __init_extra__ takes place after category initialisation). But in the case of Cython class, we simply need to look explicitly into both the class' mro and the category's parent class' mro.

+1!

For the record: this will require calling
sage.misc.structure.getattr_from_other_class, to emulate the parent
being an instance of the category's parent class.

[simon-king-jena]

comment:3

Replying to @nthiery:

For the record: this will require calling
sage.misc.structure.getattr_from_other_class, to emulate the parent
being an instance of the category's parent class.

I disagree. If I recall correctly, we look up the whole class hierarchy and execute all __init_extra__ methods that we can find. getattr_from_other_class would only return one __init_extra__, but we want all, and thus we need to go up both the Cython class' and the category's parent class' mro.

[nthiery]

comment:4

Replying to @simon-king-jena:

If I recall correctly, we look up the whole class hierarchy and execute all __init_extra__ methods that we can find.

Yes!

Still, some magic will be needed to bind each such method to the
object and call it (by default Python refuses to bind a method of a
class to a non-instance of that class); that's were
getattr_from_other_class (or some subpiece thereof) will come into
play.

[simon-king-jena]

comment:5

Replying to @nthiery:

Still, some magic will be needed to bind each such method to the
object and call it (by default Python refuses to bind a method of a
class to a non-instance of that class); that's were
getattr_from_other_class (or some subpiece thereof) will come into
play.

Do we need to bind it? We just want to call it.

[simon-king-jena]

comment:6

Replying to @simon-king-jena:

Do we need to bind it? We just want to call it.

Such as:

sage: R.<x,y> = ZZ[]
sage: C = Algebras(ZZ).parent_class
sage: C.__init_extra__.__func__(R)

[simon-king-jena]

comment:7

I just tested: When one empties the coercion cache and the coerce_from_list, then calling C.__init_extra__.__func__(R) will fill it again, which is its purpose. So, it works without binding.

[nbruin]

comment:8

Replying to @simon-king-jena:

I just tested: When one empties the coercion cache and the coerce_from_list, then calling C.__init_extra__.__func__(R) will fill it again, which is its purpose. So, it works without binding.

Well ... that works when C.__init_extra__ is an unbound method that has a __func__ attribute. Likely all unbound methods do, so you've probably dealt with the case that __init_extra__ is an unbound method. But attributes aren't necessarily! I think only unbound methods bind when called as attributes on instances. Other objects just get called. But that's a case you should test for if you want this to be properly compatible with python's inheritance. Unbound methods aren't the only things that lead to callable attributes on instances.

[nthiery]

comment:9

I guess it's fair enough to write in the specifications of the protocol that __init_extra__ shall be a method. And if some day we have a serious use case (which I kind of doubt), it will always be time to generalize the protocol.

[nbruin]

comment:10

Replying to @nthiery:

I guess it's fair enough to write in the specifications of the protocol that __init_extra__ shall be a method.

You'll have to be careful how you formulate that: cdef classes can have methods too (some custom cython object of course) and their unbound versions specifically call themselves "method" and have no __func__ attribute. They are callable, though, further suggesting one should probably check if the attribute is callable and if not, see if there is a __func__ attribute that is callable (or some safe permutation of this that performs better)

And if some day we have a serious use case (which I kind of doubt), it will always be time to generalize the protocol.

Famous last words... I've seen many cases happen in sage where little design issues (which, at the time perpetrated seemed perfectly reasonable) lead to weird, unforseen and silent failures. The problem this ticket is trying to fix is one of them.

[simon-king-jena]

comment:11

Replying to @nbruin:

You'll have to be careful how you formulate that: cdef classes can have methods too (some custom cython object of course) and their unbound versions specifically call themselves "method" and have no __func__ attribute.

Note that the idea is to go up self.__class__.mro() on the one hand, and self.category().parent_class.mro(), on the other hand. The former is no problem, since self is an instance of its class and thus we don't need the __func__ attribute. The latter is (currently, at least) no problem, since parent classes of categories are Python classes and thus have __func__ on their methods. Or am I mistaken?

[nbruin]

comment:12

Replying to @simon-king-jena:

Note that the idea is to go up self.__class__.mro() on the one hand, and self.category().parent_class.mro(), on the other hand. The former is no problem, since self is an instance of its class and thus we don't need the __func__ attribute. The latter is (currently, at least) no problem, since parent classes of categories are Python classes and thus have __func__ on their methods. Or am I mistaken?

Well, anything can be assigned to attributes in principle, regardless of whether something is a python class or not. Should we ever migrate to Python 3 then there are no unbound instancemethods at all: they are just plain functions there.

So I'd say:

• check if __init_extra__.im_func exists (this indicates an instancemethod object). If so, perhaps check if __init_extra__.im_self is None (unbound method). If it does, then call im_func.
• otherwise, just call __init_extra__.
  It's hardly more coding work and should be far more robust.

[nthiery]

comment:13

Replying to @nbruin:

So I'd say:

• check if __init_extra__.im_func exists (this indicates an instancemethod object).

or use inspect.ismethod.

[simon-king-jena]

comment:17

Ping to myself. I think the __init_extra__ can be called similar to what I did in #18756/#18758.

[simon-king-jena]

comment:18

Very strange. The following does not work, even in Python:

            sage: from sage.structure.element import RingElement
            sage: class E(RingElement):
            ....:     def __init__(self, P, n):
            ....:         self.n = n
            ....:         RingElement.__init__(self, P)
            ....:     def _repr_(self):
            ....:         return "<{}>".format(self.n)
            ....:     def _mul_(self, other):
            ....:         return self.parent()(self.n*other.n)
            ....:     def _add_(self, other):
            ....:         return self.parent()(self.n+other.n)
            ....:     def _lmul_(self, other):
            ....:         return self.parent()(self.n*other)
            sage: class P(Parent):
            ....:     Element = E
            sage: p = P(base=ZZ, category=Algebras(ZZ))
            sage: p.has_coerce_map_from(ZZ)
            False

I found that UnitalAlgebras.ParentMethods.__init_extra__ is called. However, it is claimed that the (absence of a) coerce map from ZZ has already been cached, which means that the clearly existing coercion via multiplication with p.one() is not registered.

At what point is a coercion from ZZ requested before calling __init_extra__?

[simon-king-jena]

comment:19

Shoot! One needs to do

            ....:     def _lmul_(self, other):
            ....:         return self.parent().element_class(self.parent(),self.n*other)

since self.parent()(something) apparently tries to find a coercion from something.parent() to self.parent().

[simon-king-jena]

Branch: u/SimonKing/implement_the___init_extra___protocol_of_categories_for_cython_classes_

[simon-king-jena]

Author: Simon King

[simon-king-jena]

comment:21

To make sage start, it was also needed to declare that MPolynomialRing_libsingular uses a custom coercion from the base ring. Oops, I just note that the new doctest doesn't pass. Sorry.

---

New commits:

b0f16f6

implement `__init_extra__` protocol for cython classes

[simon-king-jena]

Commit: b0f16f6

[simon-king-jena]

comment:31

Replying to @simon-king-jena:

Now tests should pass

They do, on my machine at least.

[saraedum]

comment:32

Build fails.

[saraedum]

comment:33

The changes look fine to me. It can be set to positive review once the build errors are fixed.

[jdemeyer]

comment:34

It would be nice to have an explanation of what this branch does and which problem it solves.

I would like to know why traversing the mro() this way is a reasonable solution. I don't know anything else in Python which does that. Why not use more standard Python idioms like super() calls?

[jdemeyer]

comment:35

Also: since this code seems to involve categories, why is it in Parent and not CategoryObject?

[mkoeppe]

Changed branch from u/SimonKing/implement_the___init_extra___protocol_of_categories_for_cython_classes_ to u/mkoeppe/implement_the___init_extra___protocol_of_categories_for_cython_classes_

[mkoeppe]

comment:37

Rebased on 9.2.beta3

---

New commits:

2a428fa	implement `__init_extra__` protocol for cython classes
944324d	Fix the new doctest
a221133	fix failing doctests
0ec1c06	Fix another failing test

[mkoeppe]

Changed commit from f72122d to 0ec1c06

[mkoeppe]

comment:39

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:40

Setting a new milestone for this ticket based on a cursory review.

[mkoeppe]

comment:41

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.