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

Allow parsing of arguments for cached_function and cached_method #15657

Closed
tscrim opened this issue Jan 9, 2014 · 25 comments
Closed

Allow parsing of arguments for cached_function and cached_method #15657

tscrim opened this issue Jan 9, 2014 · 25 comments

Comments

@tscrim
Copy link
Collaborator

tscrim commented Jan 9, 2014

Because it will be handy to normalize and/or ignore arguments for these functions and method. See #1314 for an example.

CC: @simon-king-jena

Component: misc

Keywords: cached method function

Author: Julian Rueth

Branch: 2f19ea5

Reviewer: Travis Scrimshaw

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

@tscrim tscrim added this to the sage-6.1 milestone Jan 9, 2014
@tscrim tscrim self-assigned this Jan 9, 2014
@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Jan 9, 2014

comment:1

(curious)

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@saraedum
Copy link
Member

Branch: u/saraedum/ticket/15657

@saraedum
Copy link
Member

comment:4

I implemented this for cached_function. I am working on something similar for cached_method.


New commits:

99be4c6Added decorator_keywords decorator to simplify writing decorators with optional keyword arguments
b74edaaAdded key_normalizer for cached_function

@saraedum
Copy link
Member

Commit: b74edaa

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 20, 2014

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

faf771bRenamed key_normalizer to create_key for cached_function to be consistent with UniqueFactory
fe7a01aAdd create_key parameter for cached_method

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 20, 2014

Changed commit from b74edaa to fe7a01a

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 26, 2014

Changed commit from fe7a01a to 275f2e1

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 26, 2014

Changed branch from u/saraedum/ticket/15657 to public/structure/cached_args-15657

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 26, 2014

comment:7

I've changed create_key into key to match with python's sorted (and it makes more sense to me). I've also moved a good part of the doc up to our current standards.

Here are some timings:

With patch:

sage: @cached_function
....: def foo():
....:     return 5
....: 
sage: %timeit foo()
1000000 loops, best of 3: 343 ns per loop

sage: class Foo:
....:     @cached_method
....:     def bar(self):
....:         return 5
....:     
sage: F = Foo()
sage: %timeit F.bar()
1000000 loops, best of 3: 235 ns per loop

sage: class FooArgs:
....:     @cached_method
....:     def bar(self, x):
....:         return x
....:     
sage: FA = FooArgs()
sage: %timeit FA.bar(5)
1000000 loops, best of 3: 1.55 µs per loop

Before:

sage: %timeit foo()
1000000 loops, best of 3: 335 ns per loop

sage: %timeit F.bar()
1000000 loops, best of 3: 250 ns per loop

sage: %timeit FA.bar(5)
1000000 loops, best of 3: 1.47 µs per loop

So there does not appear to be any (statistically) significant speed regression (I did multiple runs of each as well).

If you're happy with my changes, then positive review.

PS - I also added the key option for the cache to file.


New commits:

ec8dbf1Initial addition of a 'key' argument to cached functions.
c5e837bMerge branch 'develop' into public/structure/cached_args-15657
8d78b1bMerge branch 'develop' into public/structure/cached_args-15657
5872e55Merge branch 'develop' into public/structure/cached_args-15657
9374110Merge branch 'develop' into public/structure/cached_args-15657
03ac296Merge branch 'u/saraedum/ticket/15657' of trac.sagemath.org:sage into public/structure/cached_args-15657
275f2e1Fixes due to my bad merging skills.

@saraedum
Copy link
Member

saraedum commented Apr 2, 2014

comment:8

Sage does not start anymore for me:

ImportError: No module named terminal.interactiveshell

Somehow your merge has really gone wrong. Do you want me to fix this for you?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 2, 2014

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

2c2bed9Merge branch 'develop' into public/structure/cached_args-15657

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 2, 2014

Changed commit from 275f2e1 to 2c2bed9

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 2, 2014

comment:10

Started just fine for me with 6.2.beta5, and here's my current 6.2.beta6 version (which also starts up without error).

@saraedum
Copy link
Member

comment:11

Ok. I had to make. Now it works again. I agree with your changes.

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 10, 2014

Author: Julian Rueth

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 10, 2014

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 10, 2014

comment:12

Thanks.

@vbraun
Copy link
Member

vbraun commented Apr 14, 2014

comment:13
sage -t --long src/doc/en/thematic_tutorials/coercion_and_categories.rst  # 2 doctests failed
sage -t --long src/sage/misc/dev_tools.py  # 1 doctest failed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2014

Changed commit from 2c2bed9 to 2f19ea5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2014

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

37f1797Merge branch 'public/structure/cached_args-15657' of trac.sagemath.org:sage into public/structure/cached_args-15657
2f19ea5Fixed doctests.

@sagetrac-git sagetrac-git mannequin removed the s: positive review label Apr 18, 2014
@sagetrac-git sagetrac-git mannequin added the s: needs review label Apr 18, 2014
@tscrim
Copy link
Collaborator Author

tscrim commented Apr 18, 2014

comment:15

Fixed.

@vbraun
Copy link
Member

vbraun commented Apr 20, 2014

Changed branch from public/structure/cached_args-15657 to 2f19ea5

@saraedum
Copy link
Member

Changed commit from 2f19ea5 to none

@saraedum
Copy link
Member

comment:17

Actually using the key parameter I found a few small bugs, this is #16337.

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 13, 2014

comment:18

Hello guys !!

With designs I would like to have some new parameter for cached methods, but I wonder if it would be a good idea of slow something that we want to be fast...

Here is the thing : I would like to only cache the return values corresponding to "some" inputs of the cached function. More precisely, if some parameter is set to True I want to cache the result, otherwise I don't want to.

Do you know if it can be useful to you, or if it can be obtained by a trick with the current features of cached methods ?

Thaaaaaaanks !

Nathann

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