Opened 8 years ago

Closed 7 years ago

#12951 closed enhancement (fixed)

Support cached_methods on extension types

Reported by: SimonKing Owned by: jason
Priority: major Milestone: sage-5.9
Component: misc Keywords:
Cc: hivert, robertwb Merged in: sage-5.9.beta4
Authors: Simon King Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12215 Stopgaps:

Description

In old versions of Cython, one would have seen an error like "arbitrary decorators are not supported" when attempting to use @cached_method on the methods of a cdef class. That has now been improved. However, cached methods would still not work, because an attribute __module__ is requested that does not exist.

The aim is to work around that limitation, so that cached methods and functions genuinely work in Cython.

Attachments (1)

trac12951_cached_extension.patch (6.3 KB) - added by SimonKing 7 years ago.
Patch relative to #12215

Download all attachments as: .zip

Change History (9)

comment:1 Changed 8 years ago by SimonKing

  • Authors set to Simon King
  • Status changed from new to needs_review

As it turns out, it is still impossible to use arbitrary decorators on c(p)def functions or methods. However, the attached patch makes it possible to use the decorator on def-methods of extension classes. That used to fail, because of the attribute error mentioned in the ticket description. It is of course still required that the instances of the extension class either allow to override an attribute of the class by an attribute of the instance, or that the instance has a public attribute __cached_methods.

For the latter case, we now have (from the doc tests):

    sage: cython_code = [
    ... "from sage.misc.cachefunc import cached_method",
    ... "cdef class MyClass:",
    ... "    cdef public dict __cached_methods",
    ... "    @cached_method",
    ... "    def f(self, a,b):",
    ... "        return a*b"]
    sage: cython(os.linesep.join(cython_code))
    sage: P = MyClass()
    sage: P.f(2,3)
    6
    sage: P.f(2,3) is P.f(2,3)
    True

Note that by #11115, sage.structure.parent.Parent has the __cached_methods attribute. Hence, any cdef subclass of Parent will do!

Providing attribute access is a bit more tricky, since it is needed that an attribute inherited by the instance from its class can be overridden on the instance. That is why providing a __getattr__ would not be enough in the following example:

    sage: cython_code = [
    ... "from sage.misc.cachefunc import cached_method",
    ... "cdef class MyOtherClass:",
    ... "    cdef dict D",
    ... "    def __init__(self):",
    ... "        self.D = {}",
    ... "    def __setattr__(self, n,v):",
    ... "        self.D[n] = v",
    ... "    def __getattribute__(self, n):",
    ... "        try:",
    ... "            return self.D[n]",
    ... "        except KeyError:",
    ... "            pass",
    ... "        return getattr(type(self),n).__get__(self)",
    ... "    @cached_method",
    ... "    def f(self, a,b):",
    ... "        return a+b"]
    sage: cython(os.linesep.join(cython_code))
    sage: Q = MyOtherClass()
    sage: Q.f(2,3)
    5
    sage: Q.f(2,3) is Q.f(2,3)
    True

Supporting attribute access apparently is more complicated than the other approach, and probably not recommended. However, on cached methods, it is somehow faster than the easier method:

    sage: timeit("a = P.f(2,3)")   # random
    625 loops, best of 3: 1.3 µs per loop
    sage: timeit("a = Q.f(2,3)")   # random
    625 loops, best of 3: 931 ns per loop

Needs review!

comment:2 Changed 7 years ago by SimonKing

Any reviewer?

comment:3 Changed 7 years ago by SimonKing

  • Dependencies set to #12215

There is a conflict with #12215. Both tickets need review, but I think that #12215 is more important than this ticket. Hence, I introduced a dependency, and changed my patch accordingly.

comment:4 Changed 7 years ago by SimonKing

Note to myself: Test that this patch still applies.

Changed 7 years ago by SimonKing

Patch relative to #12215

comment:5 Changed 7 years ago by SimonKing

I had to rebase the patch. Now it should work again. Still needing review!

comment:6 Changed 7 years ago by SimonKing

  • Cc robertwb added

comment:7 Changed 7 years ago by tscrim

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

Looks good to me. Thanks Simon.

comment:8 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.9.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.