Opened 10 years ago

Closed 10 years ago

#12951 closed enhancement (fixed)

Support cached_methods on extension types

Reported by: Simon King Owned by: Jason Grout
Priority: major Milestone: sage-5.9
Component: misc Keywords:
Cc: Florent Hivert, Robert Bradshaw Merged in: sage-5.9.beta4
Authors: Simon King Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12215 Stopgaps:

Status badges

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 Simon King 10 years ago.
Patch relative to #12215

Download all attachments as: .zip

Change History (9)

comment:1 Changed 10 years ago by Simon King

Authors: Simon King
Status: newneeds_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 10 years ago by Simon King

Any reviewer?

comment:3 Changed 10 years ago by Simon King

Dependencies: #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 10 years ago by Simon King

Note to myself: Test that this patch still applies.

Changed 10 years ago by Simon King

Patch relative to #12215

comment:5 Changed 10 years ago by Simon King

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

comment:6 Changed 10 years ago by Simon King

Cc: Robert Bradshaw added

comment:7 Changed 10 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Looks good to me. Thanks Simon.

comment:8 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.9.beta4
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.