Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bind the wrapped function to the instance to which a CachedMethodCaller is bound #15056

Open
simon-king-jena opened this issue Aug 17, 2013 · 11 comments

Comments

@simon-king-jena
Copy link
Member

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.

Depends on #12601

CC: @nthiery

Component: misc

Author: Simon King

Branch/Commit: u/SimonKing/bind_the_wrapped_function_to_the_instance_to_which_a__cachedmethodcaller__is_bound @ 016518b

Issue created by migration from https://trac.sagemath.org/ticket/15056

@simon-king-jena
Copy link
Member Author

comment:1

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.

@simon-king-jena
Copy link
Member Author

comment:2

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 ...>.

@simon-king-jena
Copy link
Member Author

Author: Simon King

@simon-king-jena
Copy link
Member Author

comment:3

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

@simon-king-jena
Copy link
Member Author

comment:5

Attachment: trac15056_bind_wrapped_method.patch.gz

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.

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@fchapoton
Copy link
Contributor

comment:9

needs a git branch

@simon-king-jena
Copy link
Member Author

comment:10

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).

@simon-king-jena
Copy link
Member Author

@simon-king-jena
Copy link
Member Author

Commit: 016518b

@simon-king-jena
Copy link
Member Author

comment:12

I created a git branch manually. Needs review!


New commits:

016518bBound and unbound cached methods

@fchapoton
Copy link
Contributor

comment:13

does not apply, needs rebase

@fchapoton fchapoton removed this from the sage-6.4 milestone May 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants