Opened 8 years ago
Last modified 8 weeks ago
#15718 needs_info defect
Implement the __init_extra__ protocol of categories for Cython classes.
Reported by:  nthiery  Owned by:  

Priority:  major  Milestone:  sage9.7 
Component:  categories  Keywords:  
Cc:  Merged in:  
Authors:  Simon King  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/mkoeppe/implement_the___init_extra___protocol_of_categories_for_cython_classes_ (Commits, GitHub, GitLab)  Commit:  0ec1c066e187dafc899ab117c003059d1fdf56b1 
Dependencies:  Stopgaps: 
Description
Change History (42)
comment:1 followup: ↓ 2 Changed 8 years ago by
comment:2 in reply to: ↑ 1 ; followup: ↓ 3 Changed 8 years ago by
Replying to SimonKing:
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.
comment:3 in reply to: ↑ 2 ; followup: ↓ 4 Changed 8 years ago by
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.
comment:4 in reply to: ↑ 3 ; followup: ↓ 5 Changed 8 years ago by
Replying to SimonKing:
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 noninstance of that class); that's were
getattr_from_other_class
(or some subpiece thereof) will come into
play.
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 8 years ago by
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 noninstance 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.
comment:6 in reply to: ↑ 5 Changed 8 years ago by
Replying to SimonKing:
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)
comment:7 followup: ↓ 8 Changed 8 years ago by
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.
comment:8 in reply to: ↑ 7 Changed 8 years ago by
Replying to SimonKing:
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.
comment:9 followup: ↓ 10 Changed 8 years ago by
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.
comment:10 in reply to: ↑ 9 ; followup: ↓ 11 Changed 8 years ago by
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.
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 8 years ago by
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?
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 8 years ago by
Replying to SimonKing:
Note that the idea is to go up
self.__class__.mro()
on the one hand, andself.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 aninstancemethod
object). If so, perhaps check if__init_extra__.im_self
is None (unbound method). If it does, then callim_func
.  otherwise, just call
__init_extra__
.
It's hardly more coding work and should be far more robust.
comment:13 in reply to: ↑ 12 Changed 8 years ago by
Replying to nbruin:
So I'd say:
 check if
__init_extra__.im_func
exists (this indicates aninstancemethod
object).
or use inspect.ismethod.
comment:14 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:15 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:16 Changed 8 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:17 Changed 7 years ago by
comment:18 Changed 7 years ago by
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__
?
comment:19 Changed 7 years ago by
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()
.
comment:20 Changed 7 years ago by
 Branch set to u/SimonKing/implement_the___init_extra___protocol_of_categories_for_cython_classes_
comment:21 Changed 7 years ago by
 Commit set to b0f16f6532bc7b3bb214b0c17470fc703f5f7acc
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

comment:22 Changed 7 years ago by
 Commit changed from b0f16f6532bc7b3bb214b0c17470fc703f5f7acc to 3c36b95990f755ac80ab44e3b709240e2e751c0a
Branch pushed to git repo; I updated commit sha1. New commits:
3c36b95  Fix the new doctest

comment:23 Changed 7 years ago by
 Status changed from new to needs_review
With the additional commit, tests in sage.structure.parent pass, and I hope they do elsewhere, too.
comment:24 Changed 7 years ago by
 Status changed from needs_review to needs_work
 Work issues set to Ensure that __init_extra__ is called in the right moment
Oops:
sage t src/sage/algebras/free_algebra.py # Killed due to segmentation fault sage t src/sage/algebras/letterplace/letterplace_ideal.pyx # Killed due to segmentation fault sage t src/sage/algebras/letterplace/free_algebra_element_letterplace.pyx # Killed due to segmentation fault sage t src/sage/algebras/letterplace/free_algebra_letterplace.pyx # Killed due to segmentation fault sage t src/sage/categories/pushout.py # 5 doctests failed sage t src/sage/categories/homset.py # 4 doctests failed sage t src/sage/rings/quotient_ring_element.py # Killed due to segmentation fault sage t src/sage/rings/ring.pyx # Killed due to segmentation fault sage t src/sage/rings/quotient_ring.py # Killed due to segmentation fault sage t src/sage/rings/polynomial/polynomial_quotient_ring.py # 35 doctests failed sage t src/sage/rings/polynomial/polynomial_quotient_ring_element.py # 6 doctests failed
Sorry, I am currently unable to continue work on this. I suppose the solution is to either change the point at which Parent.__init__
is called (since it tests whether self.one()
works and multiplication works, it needs to be done rather late), or to indicate that there is a custom coercion that is better than the default coerce map (which is what I did in the case of multivariate polynomial rings).
comment:25 followup: ↓ 26 Changed 7 years ago by
I couldn't resist working on it.
In some cases, the problem is that a parent has not been given a base(ring), however it is initialised in the category of unital algebras, which means that it *has* a base(ring).
Should we perhaps modify CategoryObject.__init__
, which currently just defines self._base
and then initialises the category? It could additionally define a base for the parent, if the given category has a base.
comment:26 in reply to: ↑ 25 Changed 7 years ago by
Replying to SimonKing:
Should we perhaps modify
CategoryObject.__init__
, which currently just definesself._base
and then initialises the category? It could additionally define a base for the parent, if the given category has a base.
It looks like a good idea, but would break an awful lot of things. :(
.
comment:27 Changed 7 years ago by
 Commit changed from 3c36b95990f755ac80ab44e3b709240e2e751c0a to e6271e9121a3666727299fa61d7f78e122983e9b
Branch pushed to git repo; I updated commit sha1. New commits:
e6271e9  fix failing doctests

comment:28 Changed 7 years ago by
Now things should work.
Changes:
 For letterplace rings, one has to call the parent init later, so that the conversions that occur in
__init_extra__
work.  In pushout.py, there is one test where a class is defined as a subclass of a parentclasswithcategory. By consequence, an instance of that class is not correctly categoryinitialised.
 I just realise that I forgot some fixes for tests in homset.py. Here is problematic what I mentioned above: The base ring is not given, but the category has a base ring, and because of the missing base ring,
__init_extra__
fails. Fixing it now...
comment:29 Changed 7 years ago by
 Commit changed from e6271e9121a3666727299fa61d7f78e122983e9b to f72122d651975408670dbb6db935f280d921146d
Branch pushed to git repo; I updated commit sha1. New commits:
f72122d  Fix another failing test

comment:30 followup: ↓ 31 Changed 7 years ago by
 Status changed from needs_work to needs_review
 Work issues Ensure that __init_extra__ is called in the right moment deleted
Done! Now tests should pass (at least the previously failed tests should now be fixed...). Needs review!
comment:31 in reply to: ↑ 30 Changed 7 years ago by
comment:33 Changed 6 years ago by
The changes look fine to me. It can be set to positive review once the build errors are fixed.
comment:34 Changed 6 years ago by
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?
comment:35 Changed 6 years ago by
Also: since this code seems to involve categories, why is it in Parent
and not CategoryObject
?
comment:36 Changed 23 months ago by
 Branch changed 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_
comment:37 Changed 23 months ago by
 Commit changed from f72122d651975408670dbb6db935f280d921146d to 0ec1c066e187dafc899ab117c003059d1fdf56b1
 Milestone changed from sage6.4 to sage9.2
 Status changed from needs_work to needs_info
comment:38 Changed 22 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:39 Changed 15 months ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:40 Changed 10 months ago by
 Milestone changed from sage9.4 to sage9.5
Setting a new milestone for this ticket based on a cursory review.
comment:41 Changed 5 months ago by
 Milestone changed from sage9.5 to sage9.6
Stalled in needs_review
or needs_info
; likely won't make it into Sage 9.5.
comment:42 Changed 8 weeks ago by
 Milestone changed from sage9.6 to sage9.7
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.