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: sage-9.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:

Status badges

Description


Change History (42)

comment:1 follow-up: Changed 8 years ago by 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.

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.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by nthiery

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 ; follow-up: Changed 8 years ago by SimonKing

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 ; follow-up: Changed 8 years ago by nthiery

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 non-instance of that class); that's were getattr_from_other_class (or some subpiece thereof) will come into play.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by SimonKing

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.

comment:6 in reply to: ↑ 5 Changed 8 years ago by SimonKing

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 follow-up: Changed 8 years ago by 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.

comment:8 in reply to: ↑ 7 Changed 8 years ago by nbruin

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 follow-up: Changed 8 years ago by nthiery

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 ; follow-up: Changed 8 years ago by nbruin

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 ; follow-up: Changed 8 years ago by SimonKing

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 ; follow-up: Changed 8 years ago by nbruin

Replying to SimonKing:

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.

comment:13 in reply to: ↑ 12 Changed 8 years ago by nthiery

Replying to nbruin:

So I'd say:

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

or use inspect.ismethod.

comment:14 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:15 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:16 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:17 Changed 7 years ago by SimonKing

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

comment:18 Changed 7 years ago by SimonKing

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 SimonKing

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 SimonKing

  • Branch set to u/SimonKing/implement_the___init_extra___protocol_of_categories_for_cython_classes_

comment:21 Changed 7 years ago by SimonKing

  • Authors set to Simon King
  • 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:

b0f16f6implement __init_extra__ protocol for cython classes

comment:22 Changed 7 years ago by git

  • Commit changed from b0f16f6532bc7b3bb214b0c17470fc703f5f7acc to 3c36b95990f755ac80ab44e3b709240e2e751c0a

Branch pushed to git repo; I updated commit sha1. New commits:

3c36b95Fix the new doctest

comment:23 Changed 7 years ago by SimonKing

  • 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 SimonKing

  • 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 follow-up: Changed 7 years ago by SimonKing

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 SimonKing

Replying to SimonKing:

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.

It looks like a good idea, but would break an awful lot of things. :-(.

comment:27 Changed 7 years ago by git

  • Commit changed from 3c36b95990f755ac80ab44e3b709240e2e751c0a to e6271e9121a3666727299fa61d7f78e122983e9b

Branch pushed to git repo; I updated commit sha1. New commits:

e6271e9fix failing doctests

comment:28 Changed 7 years ago by SimonKing

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 parent-class-with-category. By consequence, an instance of that class is not correctly category-initialised.
  • 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 git

  • Commit changed from e6271e9121a3666727299fa61d7f78e122983e9b to f72122d651975408670dbb6db935f280d921146d

Branch pushed to git repo; I updated commit sha1. New commits:

f72122dFix another failing test

comment:30 follow-up: Changed 7 years ago by SimonKing

  • 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 SimonKing

Replying to SimonKing:

Now tests should pass

They do, on my machine at least.

comment:32 Changed 6 years ago by saraedum

  • Status changed from needs_review to needs_work

Build fails.

comment:33 Changed 6 years ago by saraedum

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 jdemeyer

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 jdemeyer

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

comment:36 Changed 23 months ago by mkoeppe

  • 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 mkoeppe

  • Commit changed from f72122d651975408670dbb6db935f280d921146d to 0ec1c066e187dafc899ab117c003059d1fdf56b1
  • Milestone changed from sage-6.4 to sage-9.2
  • Status changed from needs_work to needs_info

Rebased on 9.2.beta3


New commits:

2a428faimplement __init_extra__ protocol for cython classes
944324dFix the new doctest
a221133fix failing doctests
0ec1c06Fix another failing test

comment:38 Changed 22 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:39 Changed 15 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

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

comment:40 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

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

comment:41 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

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

comment:42 Changed 8 weeks ago by mkoeppe

  • Milestone changed from sage-9.6 to sage-9.7
Note: See TracTickets for help on using tickets.