Opened 8 years ago

Last modified 6 years ago

#15056 needs_work enhancement

Bind the wrapped function to the instance to which a `CachedMethodCaller` is bound

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-6.8
Component: misc Keywords:
Cc: nthiery Merged in:
Authors: Simon King Reviewers:
Report Upstream: N/A Work issues:
Branch: u/SimonKing/bind_the_wrapped_function_to_the_instance_to_which_a__cachedmethodcaller__is_bound (Commits, GitHub, GitLab) Commit: 016518b2398334101f82937e818f4e276501d205
Dependencies: #12601 Stopgaps:

Status badges

Description

Currently, a cached method will always be a wrapper of a function. It will bind a CachedMethodCaller to an instance. However, this CachedMethodCaller will not bind the wrapped function to the instance. But in some cases, we might want to make the function depend on the instance, see #12978, for example.

Therefore, I suggest to store the bound version of the wrapped function when binding a CachedMethodCaller to an instance. There might be a slight benefit timewise, too:

sage: def f(self, x): pass
sage: m = f.__get__(ZZ, type(ZZ))
sage: %timeit f(ZZ, 3)
1000000 loops, best of 3: 333 ns per loop
sage: %timeit m(3)
1000000 loops, best of 3: 320 ns per loop

I make #12601 a dependency, since it touches the same file.

Attachments (1)

trac15056_bind_wrapped_method.patch (29.1 KB) - added by SimonKing 8 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 8 years ago by SimonKing

Another nice feature would be: Have cached unbound methods, that will then be called with an additional instance. What I mean is this:

    sage: cython_code = ['cpdef test_meth(self,x):',
    ....: '    "some doc for a wrapped cython method"',
    ....: '    return -x',
    ....: 'from sage.all import cached_method',
    ....: 'from sage.structure.parent cimport Parent',
    ....: 'cdef class MyClass(Parent):',
    ....: '    @cached_method',
    ....: '    def direct_method(self, x):',
    ....: '        "Some doc for direct method"',
    ....: '        return 2*x',
    ....: '    wrapped_method = cached_method(test_meth,name="wrapped_method")']
    sage: cython(os.linesep.join(cython_code))
    sage: O = MyClass()
    sage: O.wrapped_method
    Cached version of <bound method MyClass.test_meth of <type '_home_simon__sage_temp_linux_sqwp_site_6921_tmp_xiI4E1_spyx_0.MyClass'>>
    sage: MyClass.wrapped_method
    Cached version of <unbound method MyClass.test_meth>
    sage: MyClass.wrapped_method(O, 3) is O.wrapped_method(3)
    True

I am only afraid that in the case of CachedMethodCallerNoArgs, implementing this feature might imply an inacceptable slowdown. But first experiments show that in the case of several arguments (CachedMethodCaller) things are fine.

comment:2 Changed 8 years ago by SimonKing

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

This is now ready for review (and is a prerequisite for #12978).

The underlying idea: We wrap a function f, resulting in a cached method CMf. When binding CMf to an instance inst (which can be None), then a CachedMethodCaller or CachedMethodCallerNoArgs CMCf is returned, according to the number of arguments. Now, the old behaviour was that CMCf was holding a copy of f, and was calling it passing inst as the first argument.

Now, in #12978, I envision to wrap arbitrary objects f, in particular objects with a __get__ method, so that f can be bound to inst---and obviously it is not f but f bound to inst what we want to call! As a side effect, we can distinguish cached bound methods of an instance, and cached unbound methods.

Outside of sage/misc/cachefunc.pyx, only little had to change. In fact, there have just been a few places in which the string representation of a cached method appears in a test---and the string representation has changed, because it is now Cached version of <bound/unbound method...> instead of Cached version of <function ...>.

comment:3 Changed 8 years ago by SimonKing

Argh. It doesn't work yet. Something in CachedMethodCaller._instance_call...

comment:4 Changed 8 years ago by SimonKing

  • Status changed from needs_review to needs_work

Changed 8 years ago by SimonKing

comment:5 Changed 8 years ago by SimonKing

  • Status changed from needs_work to needs_review

The problem was in the __call__ method of CachedMethodCaller in the case of a CachedInParentMethod: If we have a CachedInParentMethod then we need to tell the instance which the cached function needs to be called with.

Anyway, needs review again.

comment:6 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:7 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:8 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:9 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

needs a git branch

comment:10 Changed 6 years ago by SimonKing

I try to turn it into a git branch. Isn't git apply supposed to apply a patch? It somehow didn't work, even though it didn't give me an error message (at least when saying git apply ... in the folder sage/src).

comment:11 Changed 6 years ago by SimonKing

  • Branch set to u/SimonKing/bind_the_wrapped_function_to_the_instance_to_which_a__cachedmethodcaller__is_bound

comment:12 Changed 6 years ago by SimonKing

  • Commit set to 016518b2398334101f82937e818f4e276501d205
  • Status changed from needs_work to needs_review

I created a git branch manually. Needs review!


New commits:

016518bBound and unbound cached methods

comment:13 Changed 6 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-6.8
  • Status changed from needs_review to needs_work

does not apply, needs rebase

Note: See TracTickets for help on using tickets.