Opened 3 years ago

Closed 3 years ago

#23435 closed enhancement (fixed)

Dynamic classes of extension types should behave like extension types

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: performance Keywords:
Cc: nthiery, tscrim Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: d0f2207 (Commits) Commit: d0f220731ef334528c73484a3e13be6bc985a640
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This ticket creates dynamic classes with __slots__ = [] if all base classes of the dynamic class are extension types. This causes the dynamic class to behave like an extension type. In fact, is_extension_type() will return True for such a dynamic class.

The goal is to reduce the performance penalty when replacing an extension type by a dynamic class: extension types and Python classes with __slots__ have no __dict__ which speeds up all attribute accesses and which reduces memory usage.

Change History (18)

comment:1 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/23435

comment:2 Changed 3 years ago by jdemeyer

  • Commit set to d4fdfca8cc54d606bf54041b22bf653cd42fa4e9
  • Status changed from new to needs_review

New commits:

cf224b7Implement wrapperdescr_call without checking
9a4ef8cWording
ed0d88cMove various things to src/sage/cpython
6cb44f8New function can_assign_class()
d4fdfcaDynamic classes of extension types should behave like extension types

comment:3 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 3 years ago by git

  • Commit changed from d4fdfca8cc54d606bf54041b22bf653cd42fa4e9 to 1fc75fbfe17b1d71789e82c750e958a3a0ba60ad

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

1fc75fbDynamic classes of extension types should behave like extension types

comment:5 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 3 years ago by jdemeyer

  • Cc tscrim added
  • Status changed from needs_review to needs_work

Some doctests in src/doc/en/thematic_tutorials/tutorial-objects-and-classes.rst involving sage.combinat.free_module.CombinatorialFreeModule_with_category.element_class break.

comment:7 follow-up: Changed 3 years ago by tscrim

I am pretty sure the problem comes from a missed an is_extension_type() in __make_element_class__ in parent.pyx.

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

(Edit: I misinterpreted your comment)

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:9 Changed 3 years ago by git

  • Commit changed from 1fc75fbfe17b1d71789e82c750e958a3a0ba60ad to da56de403d18709acd9c5b780d18c267f14ac712

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

da56de4Add a __dict__ to IndexedFreeModuleElement

comment:10 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:11 Changed 3 years ago by git

  • Commit changed from da56de403d18709acd9c5b780d18c267f14ac712 to d1670e1324092d3dcb5b5297bf75e59bc47f6c29

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

d1670e1Merge tag '8.1.beta3' into t/23435/ticket/23435

comment:12 follow-up: Changed 3 years ago by tscrim

I doubt anyone is using the features provided from having a __dict__. So I think the better solution would be to change the tutorial because the CFM.element_class is really just a workaround not inheriting properly from the category implementation. Because once we remove that workaround, we would run into the same problem.

Also, IMO this test is not accurate (or at least somewhat misleading):

Some particular actions modify the data structure of ``el``::

    sage: el.rename("bla")
    sage: el
    bla

For the tests that are failing in the .. NOTE::, we can just change it to, e.g., E.<x,y> = algebras.Exterior(QQ) that has a proper Python subclass.

Does this sound okay?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by jdemeyer

Replying to tscrim:

For the tests that are failing in the .. NOTE::, we can just change it to, e.g., E.<x,y> = algebras.Exterior(QQ) that has a proper Python subclass.

Does this sound okay?

I would rather prefer to just remove those examples. I don't think it makes sense to document something as a feature when that feature depends on the implementation detail of whether the class has a __dict__. With your proposal, we would get breakage again if someone decides that algebras.Exterior should be implemented in Cython.

comment:14 in reply to: ↑ 13 Changed 3 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

For the tests that are failing in the .. NOTE::, we can just change it to, e.g., E.<x,y> = algebras.Exterior(QQ) that has a proper Python subclass.

Does this sound okay?

I would rather prefer to just remove those examples. I don't think it makes sense to document something as a feature when that feature depends on the implementation detail of whether the class has a __dict__. With your proposal, we would get breakage again if someone decides that algebras.Exterior should be implemented in Cython.

Good point. What about doing a custom class for that example, since it is about showing the difference between Python and Cython, which is a point that should be made? (Of course, I am happy to remove the "changing data structure" test.)

comment:15 Changed 3 years ago by git

  • Commit changed from d1670e1324092d3dcb5b5297bf75e59bc47f6c29 to d0f220731ef334528c73484a3e13be6bc985a640

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

e9ae79aDynamic classes of extension types should behave like extension types
d0f2207Fix documentation regarding Python/Cython classes

comment:16 Changed 3 years ago by jdemeyer

  • Dependencies #23419 deleted

comment:17 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Let it be so.

comment:18 Changed 3 years ago by vbraun

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