search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 481 forks source link

Rewrite cached_method in Cython #11115

Closed simon-king-jena closed 12 years ago

simon-king-jena commented 13 years ago

Broadening the original description of the ticket:

The aim is to provide a Cython version of the cached_method decorator. There are three benefits:

1) Speed (see timings in the comments)

2) Using cached methods in Cython code.

3) Parent and element methods of categories

Let me elaborate on the last point:

In order to make full use of the parent and element methods of a category, it should be possible to define a cached method in the category, so that any object of that category inherits it with caching.

Currently, it fails as follows:

sage: class MyCategory(Category):
....:     def super_categories(self):
....:         return [Objects()]
....:     class ParentMethods:
....:         @cached_method
....:         def f(self, x):
....:             return x^2
....:
sage: cython("""from sage.structure.parent cimport Parent
....: cdef class MyClass(Parent): pass
....: """)
sage: O = MyClass(category=MyCategory())
sage: O.f(4)
16
sage: O.f(x) is O.f(x)
False

So, the method is inherited, but not cached.

Apply:

  1. attachment: 11115_flat.patch
  2. attachment: trac_11115_docfix.patch

Note that, if you want to remove the patches after testing them, you need to do 

rm $SAGE_ROOT/devel/sage/build/sage/misc/cachefunc.*
rm $SAGE_ROOT/devel/sage/build/*/sage/misc/cachefunc.*

Otherwise, sage -br would not work.

Depends on #9138 Depends on #11900

CC: @nthiery

Component: misc

Keywords: category cython cache

Author: Simon King

Reviewer: Nicolas M. Thiéry, Andrey Novoseltsev, Volker Braun

Merged: sage-5.0.beta0

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

simon-king-jena commented 13 years ago

Changed dependencies from #9976 to #9976, #11298

simon-king-jena commented 13 years ago
comment:45

FWIW, the long tests pass for me. I really don't know what the patchbot is complaining about.

robertwb commented 13 years ago

Changed dependencies from #9976, #11298 to sage-4.7, #9976, #11298

simon-king-jena commented 13 years ago
comment:47

After investing some work on #9944, I am now resuming work on cached methods.

Let's discuss elements again. The options were:

  1. Have __cached_methods as a cdef public attribute for sage.structure.element.Element. Advantage: It allows cached methods for all elements, even if they don't accept attribute assignment. Disadvantage: A general slowdown could result if the memory footprint of an ubiquitous class increases.

  2. Do not have __cached_method for elements, but let __getattr__ test the presence of such attribute. Advantage: The footprint remains small, and cached methods can be used for classes derived from Element by simply providing them with a public attribute __cached_methods. Disadvantage: The __getattr__ method of elements needs to look for __cached_methods, which yields another slowdown.

I therefore suggest a third solution:

  1. Keep sage.structure.element.Element untouched (both the memory footprint and the __getattr__ method). Instead, introduce a new class sage.structure.element.ElementWithCache: It derives from Element and only adds a __cached_method attribute and a modified __getattr__. Hence, if the user wants an element class with the possibility to inherit cached methods or lazy attributes from the category then either

    i) the class must allow attribute assignment (class MyElement(Element) will do), or

    ii) the class must be derived from ElementWithCache instead of Element (cdef class MyElement(ElementWithCache) will do).

I am now trying to implement the third approach. It seems to me that it combines the advantages of the previous two approaches and avoids their disadvantages.

simon-king-jena commented 13 years ago

Work Issues: Cacheable subclass of Element; radical application of cached_methods

simon-king-jena commented 13 years ago
comment:48

I am now preparing a new patch that will implement my suggestion from the previous comment. Moreover, I'll radically apply cached methods.

That means the following: I will go through all Python files in sage/rings/ and sage/algebras/ defining parent structures, and I will use @cached_method on any method that has only self as an argument and will not be modified by side-effects of other methods. Any.

The reason for that project is the fact that even a simple-most possible method, such as

def is_exact(self):
    return True

takes about 690 ns on my machine, and

def prec(self):
    return self._prec

even takes about 2.1 µs.

The cached versions are much faster, 

@cached_method
def is_exact(self):
    return True
@cached_method
def prec(self):
    return self._prec

only takes about 375 ns on the same machine, with my patch.

Thus I expect to obtain a general speedup of arithmetics. The question is whether there will be a noticeable increase of memory consumption.

simon-king-jena commented 13 years ago
comment:49

I updated attachment: trac11115-cached_cython.patch: There were some doctests whose result depends on line numbers in other files. Those will break as soon as the other file will be edited. That would be a pain in the neck. I therefore removed one test that only returned the line number, and in another test I replaced the line number by "...".

And I posted a new patch attachment: trac11115-ElementWithCache.patch. With it, there will be no general support for cached methods for elements. That's to say, cached methods can be inherited from the category only if the element supports attribute assignment.

For users who really want an element class that does not allow attribute assignment but absolutely has to inherit cached methods or lazy attributes from the category, I added a class sage.structure.element.ElementWithCachedMethod. I doubt that it will be frequently used, but at least it is there.

I guess adding @cached_method on front of any small method of rings and algebras shall be on a different ticket.

Back to "needs review"!

simon-king-jena commented 13 years ago

Description changed:

--- 
+++ 
@@ -40,4 +40,8 @@

 Depends on #9976, #11298

-Apply trac11115-cached_cython.patch
+Apply:
+
+[attachment: trac11115-cached_cython.patch](https://github.com/sagemath/sage-prod/files/10652548/trac11115-cached_cython.patch.gz)
+
+[attachment: trac11115-ElementWithCache.patch](https://github.com/sagemath/sage-prod/files/10652545/trac11115-ElementWithCache.patch.gz)
simon-king-jena commented 13 years ago

Changed work issues from Cacheable subclass of Element; radical application of cached_methods to none

simon-king-jena commented 13 years ago
comment:50

I forgot to inform the patchbot:

Depends on sage-4.7, #9976, #11298

Apply trac11115-cached_cython.patch trac11115-ElementWithCache.patch

simon-king-jena commented 13 years ago
comment:51

I am currently working on different tickets. One of them, namely #11342, changes the getattr method of parents and elements. Since #11342 is more basic than this ticket, it should be applied first. Hence, I introduced it as a new dependency and updated both patches accordingly.

At least for me, all doc tests pass.

Depends on #9976, #11298, #11342

Apply trac11115-cached_cython.patch trac11115-ElementWithCache.patch

simon-king-jena commented 13 years ago

Changed dependencies from sage-4.7, #9976, #11298 to sage-4.7, #9976, #11298, 11342

simon-king-jena commented 13 years ago

Description changed:

--- 
+++ 
@@ -38,7 +38,7 @@
 So, the method is inherited, but not cached.

-Depends on #9976, #11298
+Depends on #9976, #11298, #11342

 Apply:
simon-king-jena commented 13 years ago

Changed dependencies from sage-4.7, #9976, #11298, 11342 to sage-4.7, #9976, #11298, #11342

simon-king-jena commented 13 years ago
comment:53

I had to rebase the first patch because of a change in #11298.

simon-king-jena commented 13 years ago

Support cached methods for elements only if attributes can be assigned. Provide a subclass of Element that does fully support cached methods and lazy attributes

simon-king-jena commented 13 years ago
comment:54

Attachment: trac11115-ElementWithCache.patch.gz

I have just updated the second patch, because I had some misprints in the documentation of the new class sage.structure.element.ElementWithCachedMethod.

vbraun commented 13 years ago
comment:55

trac11115-cached_cython.patch applies with fuzz 2 but seems ok. Please rediff anyways ;-)

simon-king-jena commented 13 years ago
comment:56

Replying to @vbraun:

trac11115-cached_cython.patch applies with fuzz 2

Really? I am now using sage-4.7.2.alpha2, and both patches (of course applied after the two patches from #11342) match fine:

king@mpc622:/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage$ hg qapplied
trac11342-attribute_error_message.rebased.patch
trac_11342_fix_pari_initialization.patch
king@mpc622:/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage$ hg qimport https://github.com/sagemath/sage-prod/files/10652548/trac11115-cached_cython.patch.gz
Füge trac11115-cached_cython.patch zur Seriendatei hinzu
king@mpc622:/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage$ hg qpush
Wende trac11115-cached_cython.patch an
jetzt bei: trac11115-cached_cython.patch
king@mpc622:/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage$ hg qimport https://github.com/sagemath/sage-prod/files/10652545/trac11115-ElementWithCache.patch.gz
Füge trac11115-ElementWithCache.patch zur Seriendatei hinzu
king@mpc622:/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage$ hg qpush
Wende trac11115-ElementWithCache.patch an
jetzt bei: trac11115-ElementWithCache.patch
vbraun commented 13 years ago
comment:57

I just noticed that the fuzz is because I applied your trac9138-categories_for_rings.patch first. I guess we don't consider that as a prerequisite for this ticket or for #11342 any more.

simon-king-jena commented 13 years ago
comment:58

Replying to @vbraun:

I just noticed that the fuzz is because I applied your trac9138-categories_for_rings.patch first. I guess we don't consider that as a prerequisite for this ticket or for #11342 any more.

Yes, the dependency is opposite. First #11115, then #9138.

simon-king-jena commented 13 years ago
comment:59

With sage-4.7.2.alpha2 plus #11342 and this ticket, I get a rather strange doctest error:

sage -t -long "devel/sage-main/sage/matrix/matrix2.pyx"
**********************************************************************
File "/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix2.pyx", line 7722:
    sage: M.round(10)
Expected:
    [-2.4494897428           0.0           0.0]
    [-3.6742346142  0.7071067812           0.0]
    [-4.8989794856  1.4142135624           0.0]
Got:
    [-2.4494897428           0.0           0.0]
    [-3.6742346142  0.7071067812           0.0]
    [-4.8989794856  1.4142135624          -0.0]
**********************************************************************
1 items had failures:
   1 of  68 in __main__.example_97
***Test Failed*** 1 failures.
For whitespace errors, see the file /mnt/local/king/.sage/tmp/.doctest_matrix2.py
         [22.4 s]

Hence, 0.0 becomes -0.0.

Worse is another error:

sage -t -long "devel/sage-main/sage/matrix/matrix_double_dense.pyx"
**********************************************************************
File "/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_
double_dense.pyx", line 968:
    sage: sv[2:3]
Expected:
    [2.92724029018e-16]
Got:
    [2.01161346159e-16]
**********************************************************************
File "/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 1032:
    sage: sv = A.singular_values(eps='auto'); sv
Expected:
    verbose 1 (<module>) singular values, smallest-non-zero:cutoff:largest-zero, 2.2766...:6.2421...e-14:1.4160...e-15
    [35.139963659, 2.27661020871, 0.0, 0.0]
Got:
    verbose 1 (<module>) singular values, smallest-non-zero:cutoff:largest-zero, 2.27661020871:6.2421114782e-14:8.24999265856e-16
    [35.139963659, 2.27661020871, 0.0, 0.0]
**********************************************************************
File "/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 1983:
    sage: Q
Expected:
    [ -0.359210604054   0.569326179705   0.368048420509   0.641385845805]
    [  0.179605302027  -0.144590775798   0.925041158846  -0.301884576418]
    [  0.179605302027  -0.704880032016  0.0774617736597   0.681825307224]
    [  0.898026510134   0.397624633445 -0.0532812182975   0.180566192161]
Got:
    [ -0.359210604054   0.569326179705  -0.631992205475  -0.383954808874]
    [  0.179605302027  -0.144590775798  -0.664300280125   0.711013769813]
    [  0.179605302027  -0.704880032016  -0.397049825048  -0.559676256758]
    [  0.898026510134   0.397624633445 -0.0405268611553  -0.183849426161]
**********************************************************************
File "/mnt/local/king/SAGE/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 1988:
    sage: R
Expected:
    [    -5.56776436283       2.6940795304      -2.6940795304]
    [                 0     -3.56958477752      3.56958477752]
    [                 0                  0 -9.93013661299e-16]
    [                 0                  0                  0]
Got:
    [   -5.56776436283      2.6940795304     -2.6940795304]
    [                0    -3.56958477752     3.56958477752]
    [                0                 0 4.41845043177e-16]
    [                0                 0                 0]
**********************************************************************
simon-king-jena commented 13 years ago
comment:60

I just found that #11115 is not to blame. Both failures occur when only #11342 is applied.

vbraun commented 13 years ago
comment:61

Thats x86 FPU for you! Working as intended!

simon-king-jena commented 13 years ago
comment:62

Replying to @vbraun:

Thats x86 FPU for you! Working as intended!

What does that mean?

simon-king-jena commented 13 years ago
comment:63

Arrgh. Both errors occur in UNPATCHED sage-4.7.2.alpha2.

vbraun commented 13 years ago
comment:64

Replying to @simon-king-jena:

Replying to @vbraun:

Thats x86 FPU for you! Working as intended!

What does that mean?

This means that rounding on x86 double precision is perilous at best. Standard FPU and SSE instructions have different precision, and just to make sure you can never figure out what happened there is rounding when certain register values are stored in memory. Just fix up the doctests. They were probably caused by the updated atlas spkg which we haven't tested on your architecture / compiler version...

simon-king-jena commented 13 years ago
comment:65

The errors do not occur with sage-4.7.2.alpha1 (even with my patches applied). Since alpha2 is not officially released anyway, I'll stick with alpha1 for now.

simon-king-jena commented 13 years ago
comment:66

I'd like to add one functionality: Pickling of cached functions!

Cached methods can be pickled. However, cached functions can not. Without my patch (in sage-4.6.2):

sage: from sage.databases.cunningham_tables import cunningham_prime_factors
sage: loads(dumps(cunningham_prime_factors))
---------------------------------------------------------------------------
PicklingError                             Traceback (most recent call last)

/home/king/SAGE/work/categories/objects/<ipython console> in <module>()

/mnt/local/king/SAGE/broken/local/lib/python2.6/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.dumps (sage/structure/sage_object.c:8348)()

PicklingError: Can't pickle <type 'function'>: attribute lookup __builtin__.function failed

With my patch:

sage: loads(dumps(cunningham_prime_factors))
Cached version of None

The last one is a bit grotesque... Anyway, I think it should be easy to add a an unpickling function that recovers a cached method from its name and module.

simon-king-jena commented 13 years ago

Work Issues: Pickling of cached functions

simon-king-jena commented 13 years ago
comment:67

The problem was easy to solve: The new __reduce__ method of cached functions returns an unpickling function, together with the module and the name of the cached function. The unpickling function can re-import the cached function from these data.

Example:

sage: type(cunningham_prime_factors) 
<type 'sage.misc.cachefunc.CachedFunction'> 
sage: loads(dumps(cunningham_prime_factors)) is cunningham_prime_factors #indirect doctest 
True

Apply trac11115-cached_cython.patch trac11115-ElementWithCache.patch trac11115_cached_function_pickling.patch

simon-king-jena commented 13 years ago

Description changed:

--- 
+++ 
@@ -45,3 +45,5 @@
 [attachment: trac11115-cached_cython.patch](https://github.com/sagemath/sage-prod/files/10652548/trac11115-cached_cython.patch.gz)

 [attachment: trac11115-ElementWithCache.patch](https://github.com/sagemath/sage-prod/files/10652545/trac11115-ElementWithCache.patch.gz)
+
+[attachment: trac11115_cached_function_pickling.patch](https://github.com/sagemath/sage-prod/files/10652547/trac11115_cached_function_pickling.patch.gz)
83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

Changed dependencies from sage-4.7, #9976, #11298, #11342 to #9976 #11298 #11342

simon-king-jena commented 13 years ago
comment:69

The patchbot does not understand that we have three patches. If I am not mistaken, the problem are the capital letters. Perhaps the following works?

Apply trac11115-cached_cython.patch trac11115-ElementWithCache.patch trac11115_cached_function_pickling.patch

simon-king-jena commented 13 years ago

Attachment: trac11115_element_with_cache.patch.gz

Support cached methods for elements only if attributes can be assigned. Provide a subclass of Element that does fully support cached methods and lazy attributes

simon-king-jena commented 13 years ago

Description changed:

--- 
+++ 
@@ -44,6 +44,6 @@

 [attachment: trac11115-cached_cython.patch](https://github.com/sagemath/sage-prod/files/10652548/trac11115-cached_cython.patch.gz)

-[attachment: trac11115-ElementWithCache.patch](https://github.com/sagemath/sage-prod/files/10652545/trac11115-ElementWithCache.patch.gz)
+[attachment: trac11115_element_with_cache.patch](https://github.com/sagemath/sage-prod/files/10652546/trac11115_element_with_cache.patch.gz)

 [attachment: trac11115_cached_function_pickling.patch](https://github.com/sagemath/sage-prod/files/10652547/trac11115_cached_function_pickling.patch.gz)
simon-king-jena commented 13 years ago
comment:70

Escaping the names did not work. So, I renamed one patch.

Apply trac11115-cached_cython.patch trac11115_element_with_cache.patch trac11115_cached_function_pickling.patch

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:71

Ping, to the (or potential) reviewers... (#11068, which already has positive review, depends on this one.)

Simon, perhaps re-advertise it on sage-devel.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

Reviewer: Nicolas M. Thiéry, Andrey Novoseltsev

simon-king-jena commented 13 years ago
comment:72

Replying to @nexttime:

Simon, perhaps re-advertise it on sage-devel.

AGAIN?? Would be the fourth or fifth time...

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:73

Replying to @simon-king-jena:

Replying to @nexttime:

Simon, perhaps re-advertise it on sage-devel.

AGAIN?? Would be the fourth or fifth time...

Oh, then perhaps just dig the last thread, hinting at that it still needs review...

vbraun commented 13 years ago
comment:74

I'm getting fuzz with sage-4.7.2.alpha2 + #11342. Is there a dependency missing or does it just need to be rediffed?

vbraun commented 13 years ago
comment:75

Sorry, the fuzz comes from a conflict with #9138. So you need to rediff one of the two. Since the latter is in sage-4.7.2.alpha3, how about we add it to the dependencies and rediff this ticket?

simon-king-jena commented 13 years ago

Changed work issues from Pickling of cached functions to Rebase wrt #9138

simon-king-jena commented 13 years ago
comment:76

Replying to @vbraun:

Sorry, the fuzz comes from a conflict with #9138. So you need to rediff one of the two. Since the latter is in sage-4.7.2.alpha3, how about we add it to the dependencies and rediff this ticket?

Good idea.

simon-king-jena commented 13 years ago

Changed dependencies from #9976 #11298 #11342 to #9976 #11298 #11342 #9138

simon-king-jena commented 13 years ago

Description changed:

--- 
+++ 
@@ -38,7 +38,7 @@
 So, the method is inherited, but not cached.

-Depends on #9976, #11298, #11342
+Depends on #9976, #11298, #11342, #9138

 Apply:
simon-king-jena commented 13 years ago
comment:77

Rebasing is done!

For the patchbot:

Apply trac11115-cached_cython.patch trac11115_element_with_cache.patch trac11115_cached_function_pickling.patch

simon-king-jena commented 13 years ago

Changed work issues from Rebase wrt #9138 to none

vbraun commented 13 years ago
comment:78

I still get the fuzz on top of sage-4.7.2.alpha2 + #11342 + #9138, can you double check that you got the right patch?