Sage: Ticket #11115: Rewrite cached_method in Cython
https://trac.sagemath.org/ticket/11115
<p>
Broadening the original description of the ticket:
</p>
<p>
The aim is to provide a Cython version of the cached_method decorator. There are three benefits:
</p>
<p>
1)
Speed (see timings in the comments)
</p>
<p>
2)
Using cached methods in Cython code.
</p>
<p>
3) Parent and element methods of categories
</p>
<p>
Let me elaborate on the last point:
</p>
<p>
In order to make full use of the parent and element methods of a category, it should be possible to define a <em>cached</em> method in the category, so that any object of that category inherits it <em>with caching</em>.
</p>
<p>
Currently, it fails as follows:
</p>
<pre class="wiki">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
</pre><p>
So, the method is inherited, but not cached.
</p>
<p>
<strong>Apply:</strong>
</p>
<ol><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11115/11115_flat.patch" title="Attachment '11115_flat.patch' in Ticket #11115">11115_flat.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11115/11115_flat.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11115/trac_11115_docfix.patch" title="Attachment 'trac_11115_docfix.patch' in Ticket #11115">trac_11115_docfix.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11115/trac_11115_docfix.patch" title="Download"></a>
</li></ol><p>
Note that, if you want to <strong>remove</strong> the patches after testing them, you need to do
</p>
<pre class="wiki">rm $SAGE_ROOT/devel/sage/build/sage/misc/cachefunc.*
rm $SAGE_ROOT/devel/sage/build/*/sage/misc/cachefunc.*
</pre><p>
Otherwise, <code>sage -br</code> would not work.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/11115
Trac 1.1.6SimonKingFri, 01 Apr 2011 21:54:31 GMT
https://trac.sagemath.org/ticket/11115#comment:1
https://trac.sagemath.org/ticket/11115#comment:1
<p>
The reason is as follows (compare <a class="closed ticket" href="https://trac.sagemath.org/ticket/8611" title="enhancement: speed up cached_function and cached_method (closed: fixed)">#8611</a>):
</p>
<p>
The cached method f has a getter method, that returns a <code>CachedMethodCaller</code> and tries to assign it to O as an attribute. That assignment fails. Hence, whenever f is called again, a new instance of the <code>CachedMethodCaller</code> is created. Note that before <a class="closed ticket" href="https://trac.sagemath.org/ticket/8611" title="enhancement: speed up cached_function and cached_method (closed: fixed)">#8611</a>, that was always the case (not very efficient).
</p>
<p>
Moreover, the cached method would try to cache its return values in a dictionary assigned to O - again, it fails. So, caching can not work.
</p>
<p>
My suggestion is as follows.
</p>
<p>
We want, in particular, that caching works for the base classes Element and Parent. I propose to provide them with a public cdef method <code>__cached_methods</code> (a dictionary), that is used by the getter method of the cached method to store the resulting <code>CachedMethodCaller</code>, if storing it as an attribute fails.
</p>
<p>
Concerning the cache for storing the return values of f: It can not be stored in a dict as an attribute of O. But by <a class="closed ticket" href="https://trac.sagemath.org/ticket/8611" title="enhancement: speed up cached_function and cached_method (closed: fixed)">#8611</a>, there is a dict O.f.cache that is used for storing anyway.
</p>
<p>
So, in a nutshell, it simply works (and I already have a patch that only remains to be documented).
</p>
<p>
I tried to extend that approach to <code>SageObject</code> (from which both Parent and Element inherit), but providing <code>SageObject</code> with cdef attributes will not work, since in some places there is a double inheritance, e.g., from <code>SageObject</code> and <code>list</code>.
</p>
TicketSimonKingMon, 04 Apr 2011 19:09:53 GMT
https://trac.sagemath.org/ticket/11115#comment:2
https://trac.sagemath.org/ticket/11115#comment:2
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:1" title="Comment 1">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
So, in a nutshell, it simply works (and I already have a patch that only remains to be documented).
</p>
</blockquote>
<p>
... and documentation is hard, in particular for building the reference manual. See <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>.
</p>
<p>
The idea is:
</p>
<ul><li>Modify <code>doc/common/builder.py</code> so that it uses the utilities from <code>sage.misc.sageinspect</code> rather than the utilities from the <code>inspect</code> module, and modify the utilities from <code>sage.misc.sageinspect</code> so that they work on classes and methods (currently, they only work on class instances). That's <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>.
</li><li>Use Cython for cached functions and methods -- it relies on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>, since otherwise the doc won't build correctly.
</li></ul><p>
I guess I will be able to submit patches to both tickets tomorrow.
</p>
TicketSimonKingSun, 10 Apr 2011 08:44:26 GMTstatus, description changed; author set
https://trac.sagemath.org/ticket/11115#comment:3
https://trac.sagemath.org/ticket/11115#comment:3
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=3">diff</a>)
</li>
<li><strong>author</strong>
set to <em>Simon King</em>
</li>
</ul>
<p>
It took a bit longer than I originally expected, but I think it was worth it.
</p>
<p>
<strong><span class="underline">New Features</span></strong>
</p>
<ul><li>An instance of a class deriving from Parent or Element can inherit a cached_method from its category, <em>without</em> breaking the cache, even if it does not allow attribute assignment.
</li></ul><ul><li>The cached_method decorator can, to some extent, be used in Cython code.
</li></ul><ul><li>Cached_method is a lot faster now. In fact, using the cached_method decorator on a Python class is <em>faster</em> than a hand-written cache for a Python method, provided that the arguments are given by name and not by position.
</li></ul><p>
<strong><span class="underline">Examples</span></strong>
</p>
<p>
<span class="underline">Python</span>
</p>
<p>
The following code defines a category, and a Parent class that has a method with a hand-written cache and a corresponding cached method inherited from the category:
</p>
<pre class="wiki">from sage.all import cached_method, Category, Objects, Parent
class MyCategory(Category):
def super_categories(self):
return [Objects()]
class ParentMethods:
@cached_method
def cached_by_category(self, x=100, y=-1):
return x*y
class MyPythonClass(Parent):
def __init__(self):
self._cache = {}
Parent.__init__(self, category=MyCategory())
def cached_by_python(self, x=100, y=-1):
try:
return self._cache[x,y]
except KeyError:
out = self._cache[x,y] = x*y
return out
</pre><p>
We do some sanity tests, and then show that the cached method is faster than the hand-written cache, unless we provide arguments by name:
</p>
<pre class="wiki">sage: O = MyPythonClass()
sage: O.cached_by_category() is O.cached_by_category(100) is O.cached_by_category(x=100) is O.cached_by_category(100,-1)
True
sage: O.cached_by_python(y=1) == O.cached_by_category(y=1)
True
sage: O.cached_by_python() == O.cached_by_category()
True
sage: O.cached_by_python() is O.cached_by_python(100) is O.cached_by_python(x=100) is O.cached_by_python(100,-1)
True
</pre><p>
Here are timings for the hand-knitted cache:
</p>
<pre class="wiki">sage: timeit("O.cached_by_python()", number=10^6)
1000000 loops, best of 3: 630 ns per loop
sage: timeit("O.cached_by_python(100)", number=10^6)
1000000 loops, best of 3: 970 ns per loop
sage: timeit("O.cached_by_python(y=-1)", number=10^6)
1000000 loops, best of 3: 1.1 Âµs per loop
sage: timeit("O.cached_by_python(100,-1)", number=10^6)
1000000 loops, best of 3: 1.31 Âµs per loop
</pre><p>
and here are the corresponding timings for the cached method inherited from the category:
</p>
<pre class="wiki">sage: timeit("O.cached_by_category()", number=10^6)
1000000 loops, best of 3: 314 ns per loop
sage: timeit("O.cached_by_category(100)", number=10^6)
1000000 loops, best of 3: 954 ns per loop
sage: timeit("O.cached_by_category(y=-1)", number=10^6)
1000000 loops, best of 3: 1.93 Âµs per loop
sage: timeit("O.cached_by_category(100,-1)", number=10^6)
1000000 loops, best of 3: 1.06 Âµs per loop
</pre><p>
<span class="underline">Cython</span>
</p>
<p>
You can not use arbitrary decorators in Cython. But it is now possible to wrap a Cython function by the cached_method and assign it as a method - one needs to explicitly provide its name, though. In addition, we provide a hand-written cache programmed in Cython.
</p>
<pre class="wiki">from sage.structure.parent cimport Parent
from sage.all import cached_method
cpdef test_func(self,x=100, y=-1):
return x*y
cdef class MyCythonClass(Parent):
cdef dict _cache
def __init__(self, category):
self._cache={}
Parent.__init__(self,category=category)
cached_by_decorator = cached_method(test_func, name="cached_by_decorator")
cpdef cached_by_cython(self,x=100,y=-1):
try:
return self._cache[x,y]
except KeyError:
out = self._cache[x,y] = x*y
return out
</pre><p>
It is a Parent class, and thus it can inherit parent methods from a category. Without the patch, the cache of an inherited cached_method would break, but now it is fine:
</p>
<pre class="wiki">sage: C = MyCythonClass(MyCategory())
sage: C.cached_by_category(y=-1) is C.cached_by_category(100,-1)
True
sage: C.cached_by_decorator(y=-1) is C.cached_by_decorator(100,-1)
True
sage: C.cached_by_decorator(y=-1) == C.cached_by_category(100,-1)
True
</pre><p>
The trick is that I introduced an attribute <code>__cached_methods</code> for Parent and Element, in which a cached method can be stored. The cache is (since <a class="closed ticket" href="https://trac.sagemath.org/ticket/8611" title="enhancement: speed up cached_function and cached_method (closed: fixed)">#8611</a>) stored as an attribute of the bound cached method.
</p>
<p>
While it is nice that cached_method works at all in Cython, the performance is not as good as I wish. Here are the times for the hand-knitted cache written in Cython:
</p>
<pre class="wiki">sage: timeit("C.cached_by_cython()", number=10^6)
1000000 loops, best of 3: 242 ns per loop
sage: timeit("C.cached_by_cython(100)", number=10^6)
1000000 loops, best of 3: 538 ns per loop
sage: timeit("C.cached_by_cython(y=-1)", number=10^6)
1000000 loops, best of 3: 750 ns per loop
sage: timeit("C.cached_by_cython(100,-1)", number=10^6)
1000000 loops, best of 3: 882 ns per loop
</pre><p>
Here for the cached_method inherited from the category:
</p>
<pre class="wiki">sage: timeit("C.cached_by_category()", number=10^6)
1000000 loops, best of 3: 754 ns per loop
sage: timeit("C.cached_by_category(100)", number=10^6)
1000000 loops, best of 3: 1.62 Âµs per loop
sage: timeit("C.cached_by_category(y=-1)", number=10^6)
1000000 loops, best of 3: 2.77 Âµs per loop
sage: timeit("C.cached_by_category(100,-1)", number=10^6)
1000000 loops, best of 3: 1.76 Âµs per loop
</pre><p>
And here using the decorator in Cython code:
</p>
<pre class="wiki">sage: timeit("C.cached_by_decorator()", number=10^6)
1000000 loops, best of 3: 421 ns per loop
sage: timeit("C.cached_by_decorator(100)", number=10^6)
1000000 loops, best of 3: 1.02 Âµs per loop
sage: timeit("C.cached_by_decorator(y=-1)", number=10^6)
1000000 loops, best of 3: 1.96 Âµs per loop
sage: timeit("C.cached_by_decorator(100,-1)", number=10^6)
1000000 loops, best of 3: 1.07 Âµs per loop
</pre><p>
<strong><span class="underline">Conclusion</span></strong>
</p>
<p>
The patch provides a considerable speed-up in a case where cached_method used before, so that cached_method is not only convenient but efficient. Also, it enables to use cached_method in a broader context; here, it is not particularly efficient, but it is convenient.
</p>
<p>
We want that decorated (in particular, cached) methods appear nicely in introspection and in the reference manual. Therefore, I like to have:
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>
</p>
TicketSimonKingSun, 10 Apr 2011 19:41:46 GMTdescription, summary changed
https://trac.sagemath.org/ticket/11115#comment:4
https://trac.sagemath.org/ticket/11115#comment:4
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=4">diff</a>)
</li>
<li><strong>summary</strong>
changed from <em>Allow for extension classes to inherit cached methods from their category</em> to <em>Rewrite cached_method in Cython</em>
</li>
</ul>
TicketnovoseltSun, 10 Apr 2011 23:02:25 GMT
https://trac.sagemath.org/ticket/11115#comment:5
https://trac.sagemath.org/ticket/11115#comment:5
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:1" title="Comment 1">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
I tried to extend that approach to <code>SageObject</code> (from which both Parent and Element inherit), but providing <code>SageObject</code> with cdef attributes will not work, since in some places there is a double inheritance, e.g., from <code>SageObject</code> and <code>list</code>.
</p>
</blockquote>
<p>
What exactly is the problem with double inheritance? It would be nice if all <code>SageObjects</code> had a fast uniform cache...
</p>
TicketSimonKingMon, 11 Apr 2011 05:39:37 GMT
https://trac.sagemath.org/ticket/11115#comment:6
https://trac.sagemath.org/ticket/11115#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:5" title="Comment 5">novoselt</a>:
</p>
<blockquote class="citation">
<p>
...
What exactly is the problem with double inheritance? It would be nice if all <code>SageObjects</code> had a fast uniform cache...
</p>
</blockquote>
<p>
The problem is that you can not have double inheritance from extension classes if Python does not know whether the underlying data structures are compatible. So, if you have
</p>
<pre class="wiki">cdef class SageObject:
pass
</pre><p>
(which is currently the case) then it is fine, such as here:
</p>
<pre class="wiki">sage: cython('cdef class MySimpleObject: pass')
sage: class C(list,MySimpleObject): pass
....:
</pre><p>
But you get an error if you want to put more structure in it
</p>
<pre class="wiki">sage: cython('''cdef class MyObject:
....: cdef dict __cached_methods
....: ''')
sage: class C(list,MyObject): pass
....:
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
/mnt/local/king/SAGE/broken/devel/sage-main/<ipython console> in <module>()
TypeError: Error when calling the metaclass bases
multiple bases have instance lay-out conflict
</pre>
TicketSimonKingTue, 12 Apr 2011 09:59:34 GMT
https://trac.sagemath.org/ticket/11115#comment:7
https://trac.sagemath.org/ticket/11115#comment:7
<p>
Here is another data point:
</p>
<pre class="wiki">sage: class MyClass:
....: def __init__(self, v):
....: self.__v = v
....: def v(self):
....: return self.__v
....: @cached_method
....: def cached_v(self):
....: return self.__v
....: def derived_from_underscore(self):
....: x = self.__v^2
....: def derived_from_cache(self):
....: x = self.cached_v()^2
....:
sage: O = MyClass(100)
sage: O.v()
100
sage: O.cached_v()
100
sage: timeit('O.v()')
625 loops, best of 3: 599 ns per loop
sage: timeit('O.cached_v()')
625 loops, best of 3: 425 ns per loop
</pre><p>
There are methods that are frequently called and just return a double-underscore attribute. It would be faster to achieve the same with a cached method! I guess this is because of the name mangling that takes place for double-underscore attributes.
</p>
<p>
However, that only holds when calling a method. Inside of a method, the double-underscore seems to be quicker:
</p>
<pre class="wiki">sage: timeit('O.derived_from_underscore()')
625 loops, best of 3: 1.65 µs per loop
sage: timeit('O.derived_from_cache()')
625 loops, best of 3: 1.98 µs per loop
</pre><p>
I think it would be worth trying to use this trick: Little methods returning a double-underscore attribute appear all over the place. See <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> for one example.
</p>
TicketSimonKingWed, 13 Apr 2011 15:26:40 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:8
https://trac.sagemath.org/ticket/11115#comment:8
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
There is one problem: The startup time increases.
</p>
<p>
I suspect this is because I extended the <code>__getattr__</code> methods of parents and elements, so that it became slower -- slightly, I thought, but apparently noticeable. The original reason for doing so was an attempt to make a cached method available by attribute access, rather than by means of the <code>__get__</code> methods of the <code>CachedMethod</code> class. I have to think of something better.
</p>
TicketSimonKingThu, 14 Apr 2011 07:25:42 GMT
https://trac.sagemath.org/ticket/11115#comment:9
https://trac.sagemath.org/ticket/11115#comment:9
<p>
I did some tests. I reverted the <code>__getattr__</code> methods, so that the only change was that <code>CachedMethod</code> was not a Python but a Cython class. But I still saw an increased startup time.
</p>
<p>
That is strange. Is it the case that loading a Cython class takes longer than loading a Python class.
</p>
TicketSimonKingThu, 14 Apr 2011 07:41:44 GMT
https://trac.sagemath.org/ticket/11115#comment:10
https://trac.sagemath.org/ticket/11115#comment:10
<p>
One more idea: Currently, a cached method will modify its own <code>__doc__</code> attribute, and some care is taken that it gets the correct one. Is it really needed to store the documentation? After all, it can easily be retrieved from the wrapped function or method!
</p>
<p>
Certainly it does not happen very frequently that the documentation of a method (cached or not) is requested: It is when the documentation is built, and it is when the user wants to learn something.
</p>
<p>
So, computing and storing the documentation during initialisation of a cached method may be a waste of time and also a waste of memory. Instead, I suggest that the documentation is only computed in the method <code>self._sage_doc_()</code>. I am now testing if that helps, startuptimewise.
</p>
TicketSimonKingThu, 14 Apr 2011 12:59:36 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:11
https://trac.sagemath.org/ticket/11115#comment:11
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I think the problem with the startup time is solved. Indeed, it turned out that the time was wasted by creating doc strings for cached methods.
</p>
<p>
Next, I'll do some more tests. If it can be confirmed that a Python method returning a double-underscore attribute is slower than a cached_method that does the same, then I will try to speed Sage up in general, by using cached methods extensively. I wonder what will come out...
</p>
<p>
Still:
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>
</p>
TicketSimonKingFri, 15 Apr 2011 16:59:50 GMT
https://trac.sagemath.org/ticket/11115#comment:12
https://trac.sagemath.org/ticket/11115#comment:12
<p>
In order to be on the safe side, I made some timings for the patches from <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> (on top of <a class="closed ticket" href="https://trac.sagemath.org/ticket/9944" title="defect: categories for polynomial rings (closed: fixed)">#9944</a>), and I'd like to repeat these timings here, with this patch applied additionally.
</p>
<pre class="wiki">$ ./sage -startuptime
...
== Slowest (including children) ==
1.244 sage.all (None)
0.280 sage.schemes.all (sage.all)
0.243 sage.misc.all (sage.all)
0.162 elliptic_curves.all (sage.schemes.all)
...
</pre><p>
So, there is no slow-down, compared with the timings from an unpatched Sage on the same machine, as reported on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>.
</p>
<p>
Here is an arithmetical example:
</p>
<pre class="wiki">sage: B.<t> = PolynomialRing(Integers(125))
sage: R = monsky_washnitzer.SpecialCubicQuotientRing(t^3 - t + B(1/4))
sage: x, T = R.gens()
sage: timeit('x*T')
625 loops, best of 3: 893 µs per loop
</pre><p>
That is about the same as with the patches from <a class="closed ticket" href="https://trac.sagemath.org/ticket/9944" title="defect: categories for polynomial rings (closed: fixed)">#9944</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>, but slower than with an unpatched Sage.
</p>
<p>
I found that one can use the improved cached_methods to considerably speed up the arithmetics. Namely, in the multiplication, the method <code>modulus()</code> from sage.rings.polynomial.polynomial_ring is called repeatedly. The <code>modulus()</code> method returns ´self.basering().characteristic()`. When I put that method under a cached_method decorator, the timing looks much better:
</p>
<pre class="wiki">sage: B.<t> = PolynomialRing(Integers(125))
sage: R = monsky_washnitzer.SpecialCubicQuotientRing(t^3 - t + B(1/4))
sage: x, T = R.gens()
sage: timeit('x*T')
625 loops, best of 3: 253 µs per loop
</pre><p>
That is much faster than without patches, which is 501 µs!!.
</p>
<p>
Currently, I try to find some places where using a cached_method might be beneficial. As I have demonstrated in previous posts, there are chances to speed methods up that do nothing more but "<code>return self.__some_argument</code>".
</p>
<p>
But independent of that possible second patch, I think the first patch can be reviewed.
</p>
TicketSimonKingMon, 18 Apr 2011 16:16:37 GMT
https://trac.sagemath.org/ticket/11115#comment:13
https://trac.sagemath.org/ticket/11115#comment:13
<p>
I improved the performance of accessing the cache even further.
</p>
<p>
It is quite common to have methods that do not take arguments but whose return value should be cached. Typical example is the set of generators of a multivariate polynomial ring. Since sage-4.7.alpha5, it is a cached method.
</p>
<p>
Obviously, if a method takes no arguments, then caching the single value is much easier than storing the results in a dictionary for different arguments. I implemented this special case in a class <code>CachedMethodCallerNoArgs</code>. It is automatically used if <code>@cached_method</code> is a applied to a method without arguments. In particular, one does not need to do changes in the code.
</p>
<p>
Here is the performance. Bot <code>I.groebner_basis</code> and <code>I.gens</code> are cached methods (you need to take sage-4.7.alpha5 for the example):
</p>
<pre class="wiki">sage: P.<a,b,c,d> = QQ[]
sage: I = P*[a,b]
sage: I.gens()
[a, b]
sage: I.groebner_basis()
[a, b]
sage: type(I.gens)
<type 'sage.misc.cachefunc.CachedMethodCallerNoArgs'>
sage: type(I.groebner_basis)
<type 'sage.misc.cachefunc.CachedMethodCaller'>
sage: timeit('I.gens()',number=10^6)
1000000 loops, best of 3: 170 ns per loop
sage: timeit('I.groebner_basis()',number=10^6)
1000000 loops, best of 3: 250 ns per loop
</pre><p>
That is much faster than with an unpatched sage-4.7.alpha5:
</p>
<pre class="wiki">sage: P.<a,b,c,d> = QQ[]
sage: I = P*[a,b]
sage: I.groebner_basis()
[a, b]
sage: I.gens()
[a, b]
sage: timeit('I.gens()',number=10^6)
1000000 loops, best of 3: 699 ns per loop
sage: timeit('I.groebner_basis()',number=10^6)
1000000 loops, best of 3: 746 ns per loop
</pre><p>
To give you an idea of how fast 170 ns or 250 ns are:
</p>
<pre class="wiki">sage: class MyClass:
....: def __init__(self):
....: self._v = 10
....: self.__v = 10
....: def m0(self):
....: return 10
....: def m1(self):
....: return self._v
....: def m2(self):
....: return self.__v
....:
sage: O = MyClass()
sage: timeit('O.m0()')
625 loops, best of 3: 1.01 µs per loop
sage: timeit('O.m1()')
625 loops, best of 3: 622 ns per loop
sage: timeit('O.m2()')
625 loops, best of 3: 621 ns per loop
</pre><p>
Note that the cache is pickled -- see examples in the doc strings.
</p>
<p>
There is one further extension. There is a class <code>sage.misc.cachefunc.ClearCacheOnPickle</code>, whose purpose is quite nice: If you have a class that inherits from <code>clearCacheOnPickle</code> then the cached values will <em>not</em> be pickled.
</p>
<p>
That is to say: Let X be an instance of <a class="missing wiki">ClearCacheOnPickle?</a> and assume that its pickling is implemented using the <code>__get_newargs__</code> and <code>__get_state__</code> mantra. Then, the cache of any cached method of <code>loads(dumps(X))</code> will be empty (but of course the cache of X is preserved).
</p>
<p>
The idea existed, but it did not work for me. So, I re-implemented it essentially from scratch, using the original ideas. Also, I provided doc tests for <code>ClearCacheOnPickle</code>; there had been none before.
</p>
<p>
On top of sage-4.7.alpha5, I have the patches from <a class="closed ticket" href="https://trac.sagemath.org/ticket/10296" title="enhancement: Singular interface wasting time by waiting for the prompt too often (closed: fixed)">#10296</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/9944" title="defect: categories for polynomial rings (closed: fixed)">#9944</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>, and then all doc tests pass with the patch from here. But there should only be one dependency, namely of <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>, which provides introspection to interactive Cython code.
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>
</p>
TicketSimonKingTue, 19 Apr 2011 07:50:43 GMT
https://trac.sagemath.org/ticket/11115#comment:14
https://trac.sagemath.org/ticket/11115#comment:14
<p>
I just updated the patch, because I noticed the following: If you have an old pickle of an object with a cached method then of course the pickle will contain the cached value. If you unpickle then it may be that an old <code>CachedMethodCaller</code> becomes a new <code>CachedMethodCallerNoArgs</code>.
</p>
<p>
It would thus be nice if <code>CachedMethodCallerNoArgs</code> could retrieve the cache value of an old pickled <code>CachedMethodCaller</code>. This is implemented in the new patch.
</p>
<p>
What I did to test it:
</p>
<ol><li>Without the patch applied, do
<pre class="wiki">sage: P.<a,b,c,d> = QQ[]
sage: I = P*[a,b]
sage: I.gens() # this is in order to fill the cache - it is a CachedMethodCaller
sage: save(I,'pickletest')
</pre></li></ol><ol start="2"><li>With the patch applied, do
<pre class="wiki">sage: I = load('pickletest')
sage: I.gens.cache # this is now a CachedMethodCallerNoArgs
[a, b]
</pre></li></ol><p>
So, the cache is already filled after unpickling.
</p>
TicketSimonKingWed, 20 Apr 2011 08:49:42 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/11115#comment:15
https://trac.sagemath.org/ticket/11115#comment:15
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>Wait for #9138</em>
</li>
</ul>
<p>
While trying to fix doc tests, I noticed that there are too many non-initialised rings left that are not covered by the patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>. I think it is more clean to fix it there, not here. Since <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> needs work, this needs work as well.
</p>
TicketnthieryWed, 20 Apr 2011 09:15:04 GMT
https://trac.sagemath.org/ticket/11115#comment:16
https://trac.sagemath.org/ticket/11115#comment:16
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:13" title="Comment 13">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Obviously, if a method takes no arguments, then caching the single value is much easier than storing the results in a dictionary for different arguments. I implemented this special case in a class <code>CachedMethodCallerNoArgs</code>. It is automatically used if <code>@cached_method</code> is a applied to a method without arguments. In particular, one does not need to do changes in the code.
</p>
</blockquote>
<p>
Yeah! I have been dreaming of this performance improvement since 2008!
</p>
<p>
Altogether, my only reluctance on this patch is about the addition of
an attribute to all Sage elements. This sounds like a non trivial
increase of the memory footprint for small objects like finite field
elements. I don't have an opinion about this myself, but this
definitely requires a discussion on sage-devel.
</p>
<p>
Parents are big objects, so this is not an issue for those.
</p>
<p>
Simon: you are doing way too much good stuff for me to follow!
Honestly, I feel a bit lost about where to start. Do you mind making a
prioritized list of patches that need review, say on the category
roadmap [1]?
</p>
<p>
Thanks!
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
<p>
[1] <a class="ext-link" href="http://trac.sagemath.org/sage_trac/wiki/CategoriesRoadMap"><span class="icon"></span>http://trac.sagemath.org/sage_trac/wiki/CategoriesRoadMap</a>
</p>
TicketSimonKingWed, 20 Apr 2011 10:01:17 GMT
https://trac.sagemath.org/ticket/11115#comment:17
https://trac.sagemath.org/ticket/11115#comment:17
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:16" title="Comment 16">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:13" title="Comment 13">SimonKing</a>:
Altogether, my only reluctance on this patch is about the addition of
an attribute to all Sage elements.
</p>
</blockquote>
<p>
I did that for symmetry: Parents and Elements both accept cached methods. Cached methods only work for an object O if it either accepts attribute assignment, or provides a special location that is inspected by <code>__getattr__</code>. Otherwise, you need to reconstruct the <code>CachedMethodCaller</code> over and over again, which is a waste of time and also means that, e.g., "R.zero_ideal" would always be a <em>different</em> instance. Thus, "R.zero_ideal.cache" (which then also can not be stored as an attribute of R) is always different as well: The cache would break.
</p>
<p>
But of course, we could argue that cached methods are not of central importance for elements. So, we may decide to restrict support of cached methods to those elements that allow for attribute assignment. Then, we could remove the new <code>__cached_methods</code> attribute from sage.structure.element.Element. I could live with that restricted use of cached methods for elements.
</p>
<blockquote class="citation">
<p>
This sounds like a non trivial
increase of the memory footprint for small objects like finite field
elements.
</p>
</blockquote>
<p>
If no cached methods are used (more precisely: if they are not called), then <code>__cached_methods</code> should just be None (i.e., nothing more but a pointer, if I understand correctly). So, that's one word per object. If you do call cached methods then of course you need to store them somewhere.
</p>
<blockquote class="citation">
<p>
Simon: you are doing way too much good stuff for me to follow!
Honestly, I feel a bit lost about where to start. Do you mind making a
prioritized list of patches that need review, say on the category
roadmap [1]?
</p>
</blockquote>
<p>
My main aim is the letterplace wrapper <a class="closed ticket" href="https://trac.sagemath.org/ticket/7797" title="enhancement: Full interface to letterplace from singular (closed: fixed)">#7797</a>.
</p>
<p>
It needs support for ideals in non-commutative rings. That is <a class="closed ticket" href="https://trac.sagemath.org/ticket/11068" title="enhancement: Basic implementation of one- and twosided ideals of non-commutative ... (closed: fixed)">#11068</a>.
</p>
<p>
Since there are rings that do not inherit from sage.rings.ring.Ring, we need to move (or duplicate?) methods from there in sage.categories.rings.Rings.<a class="missing wiki">ParentMethods?</a>. That includes cached methods -- but there are rings that do not allow attribute assignment. This is why I originally opened this patch, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> (in the beginning, it was not so much about performance).
</p>
<p>
In order to have cached methods (in particular the argspec) appear nicely in the documentation, we need <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>, which already has a positive review.
</p>
<p>
Moving stuff from sage.rings.ring.Ring to sage.categories.rings.Rings.<a class="missing wiki">ParentMethods?</a> instead of duplicating can only work if <em>all</em> rings use the category framework. That's the purpose of <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>, with a small part already done in <a class="closed ticket" href="https://trac.sagemath.org/ticket/9944" title="defect: categories for polynomial rings (closed: fixed)">#9944</a>.
</p>
<p>
So, my personal roadmap is simply:
</p>
<pre class="wiki"> #9944 #9976
#9138 #11115
#11068
#7797
</pre><p>
After that, I can finally start with my main project: Compute Ext algebras for finite dimensional path algebra quotients.
</p>
TicketSimonKingWed, 20 Apr 2011 10:17:37 GMT
https://trac.sagemath.org/ticket/11115#comment:18
https://trac.sagemath.org/ticket/11115#comment:18
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:16" title="Comment 16">nthiery</a>:
</p>
<blockquote class="citation">
<p>
[1] <a class="ext-link" href="http://trac.sagemath.org/sage_trac/wiki/CategoriesRoadMap"><span class="icon"></span>http://trac.sagemath.org/sage_trac/wiki/CategoriesRoadMap</a>
</p>
</blockquote>
<p>
I inserted my favourite tickets into the section "Later patches related with or using categories:" (renaming that section, since not all of them <em>use</em> categories, but all of them <em>relate with</em> the category framework).
</p>
<p>
The tickets depend on each other, thus, the priorities are implicit.
</p>
TicketnthieryWed, 20 Apr 2011 11:51:48 GMT
https://trac.sagemath.org/ticket/11115#comment:19
https://trac.sagemath.org/ticket/11115#comment:19
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:17" title="Comment 17">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:16" title="Comment 16">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:13" title="Comment 13">SimonKing</a>:
Altogether, my only reluctance on this patch is about the addition of
an attribute to all Sage elements.
</p>
</blockquote>
<p>
I did that for symmetry: ...
</p>
</blockquote>
<p>
Yes, I definitely see the motivations and benefits! I am just saying
that adding one word to all elements is not a light decision that we
can take unilaterally.
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
TicketnthieryWed, 20 Apr 2011 11:57:56 GMT
https://trac.sagemath.org/ticket/11115#comment:20
https://trac.sagemath.org/ticket/11115#comment:20
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:18" title="Comment 18">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:16" title="Comment 16">nthiery</a>:
</p>
<blockquote class="citation">
<p>
[1] <a class="ext-link" href="http://trac.sagemath.org/sage_trac/wiki/CategoriesRoadMap"><span class="icon"></span>http://trac.sagemath.org/sage_trac/wiki/CategoriesRoadMap</a>
</p>
</blockquote>
<p>
I inserted my favourite tickets into the section "Later patches related with or using categories:" (renaming that section, since not all of them <em>use</em> categories, but all of them <em>relate with</em> the category framework).
</p>
<p>
The tickets depend on each other, thus, the priorities are implicit.
</p>
</blockquote>
<p>
Thanks! By the way, did you include there all the patches about morphisms?
</p>
<blockquote class="citation">
<p>
After that, I can finally start with my main project: Compute Ext
algebras for finite dimensional path algebra quotients.
</p>
</blockquote>
<p>
:-)
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
TicketSimonKingWed, 20 Apr 2011 12:14:44 GMT
https://trac.sagemath.org/ticket/11115#comment:21
https://trac.sagemath.org/ticket/11115#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:20" title="Comment 20">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Thanks! By the way, did you include there all the patches about morphisms?
</p>
</blockquote>
<p>
I completely forgot about them, and probably I would not be able to work of them in the near future (1 month or so).
</p>
TicketSimonKingMon, 25 Apr 2011 17:55:48 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/11115#comment:22
https://trac.sagemath.org/ticket/11115#comment:22
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_info</em>
</li>
<li><strong>work_issues</strong>
<em>Wait for #9138</em> deleted
</li>
</ul>
<p>
I am about to ask sage-devel about introducing an additional attribute to elements. <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> should be fine now, so, I change the status from "needs work" to "needs info" (from sage-devel).
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>
</p>
TicketSimonKingTue, 26 Apr 2011 16:58:42 GMT
https://trac.sagemath.org/ticket/11115#comment:23
https://trac.sagemath.org/ticket/11115#comment:23
<p>
Unfortunately, there was no reaction from sage-devel, and my question has already disappeared from the first screen.
</p>
<p>
I see an alternative to a new attribute <code>__cached_methods</code> for all elements: Since that attribute will only be used if the element does not allow attribute assignment, and since only few element classes use cached methods, it might be a good compromise to introduce the <code>__cached_methods</code> attribute only in those derived classes that really need it.
</p>
<p>
Hence, you would write
</p>
<pre class="wiki">cdef class MyElementClass(Element):
cdef public dict __cached_methods
cdef ...
def __init__(...)
</pre><p>
if <code>MyElementClass</code> does not accept attribute assignment but is supposed to inherit cached methods from the category framework.
</p>
<p>
I do suggest to keep the additional attribute for sage.structure.parent.Parent, though. As you said, parents are big and rare enough that an additional attribute is no real burden.
</p>
TicketSimonKingWed, 27 Apr 2011 19:18:18 GMT
https://trac.sagemath.org/ticket/11115#comment:24
https://trac.sagemath.org/ticket/11115#comment:24
<p>
It occured to me that I did not yet provide an example to show cached methods for elements that do not allow attribute assignment:
</p>
<pre class="wiki">sage: cython("""from sage.structure.element cimport Element
cdef class MyElement(Element):
cdef object x
def __init__(self,P,x):
self.x=x
Element.__init__(self,P)
def __neg__(self):
return MyElement(self.parent(),-self.x)
def _repr_(self):
return '<%s>'%self.x
def raw_test(self):
return -self
""")
sage: class MyCategory(Category):
@cached_method
def super_categories(self):
return [Objects()]
class ElementMethods:
@cached_method
def test_cache(self):
return -self
sage: class MyParent(Parent): pass
sage: C = MyCategory()
sage: P = MyParent(category=C)
sage: e = MyElement(P,5)
sage: e
<5>
sage: -e
<-5>
# This is inherited from the category, ...
sage: e.test_cache()
<-5>
# ... and the cache works, ...
sage: e.test_cache() is e.test_cache()
True
# ... even though you can't set attributes:
sage: e.bla = 1
Traceback (most recent call last):
...
AttributeError: '_mnt_local_king__sage_temp_mpc622_25602_tmp_1_spyx_0.MyElement' object has no attribute 'bla'
# By lack of "setattr", the cache is not as fast as it could be, ...
sage: timeit('e.test_cache()')
625 loops, best of 3: 1.62 Âµs per loop
# ... but it is faster than to create a new element
sage: timeit('e.raw_test()')
625 loops, best of 3: 1.78 Âµs per loop
</pre><p>
At sage-devel, Martin Raum suggested to time element creation (if I understood his suggestion correctly).
</p>
<p>
I found without patch:
</p>
<pre class="wiki">sage: K = GF(101)
sage: get_memory_usage()
839.96484375
sage: %time for i in xrange(10^7): a = K(i)
CPU times: user 25.19 s, sys: 0.01 s, total: 25.21 s
Wall time: 25.20 s
sage: get_memory_usage()
839.96484375
</pre><p>
and with patch
</p>
<pre class="wiki">sage: K = GF(101)
sage: get_memory_usage()
841.96875
# show that the new attribute is there, but it is non-initialized
sage: a=K(4)
sage: print a.__cached_methods
None
sage: %time for i in xrange(10^7): a = K(i)
CPU times: user 24.46 s, sys: 0.00 s, total: 24.46 s
Wall time: 24.46 s
sage: get_memory_usage()
841.96875
</pre><p>
So the time actually improved with the patch, and the memory usage at starting sage increased by as much as 0.24% - I hope that can be afforded.
</p>
TicketSimonKingThu, 28 Apr 2011 08:17:26 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:25
https://trac.sagemath.org/ticket/11115#comment:25
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
There was rather little feedback on sage-devel.
</p>
<p>
The example above shows that the increase of memory consumption - at least directly after starting sage - is certainly tolerable. The performance gain is obvious.
</p>
<p>
There remains the question what we shall do with elements versus element class of a category. Options:
</p>
<blockquote>
<p>
1) Do what the patch suggest. Then, every instance of Element can inherit a cached method from the element class of a category, regardless whether it accepts attribute assignment or not. The cached method will be more efficient if you can assign attributes, but at least the cache won't break.
</p>
</blockquote>
<blockquote>
<p>
2) Do not support the inheritance of cached methods from the category to elements that don't support attribute assignment.
</p>
</blockquote>
<blockquote>
<p>
3) A combination of 1) and 2): If the user defines an attribute <code>__cached_methods</code> for its element, then it could inherit cached methods from the element class of a category.
</p>
</blockquote>
<p>
I already stated what I'd prefer.
</p>
<p>
By the way, I just noticed that elements can currently (with or without the patch) <em>not</em> inherit a cached_in_parent_method. Reason:
</p>
<pre class="wiki">sage: class MyCategory(Category):
....: def super_categories(self):
....: return [Objects()]
....: class ElementMethods:
....: @cached_in_parent_method
....: def test_cache(self):
....: return -self
....:
sage: C = MyCategory()
sage: C.element_class.test_cache
Traceback (most recent call last):
...
AttributeError: 'NoneType' object has no attribute 'parent'
</pre><p>
I think I could make it work. Should I do that here, or better on a different ticket (which depends on this)?
</p>
TicketSimonKingThu, 28 Apr 2011 08:36:18 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:26
https://trac.sagemath.org/ticket/11115#comment:26
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
It turned out that the change is very small. So, I'll add a doc test and then submit a new patch here.
</p>
TicketSimonKingThu, 28 Apr 2011 09:13:36 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:27
https://trac.sagemath.org/ticket/11115#comment:27
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
The new patch is posted. Apart from fixing the categorical trouble with cached_in_parent methods, it turns several doc strings into raw strings (they contain backslashes). Please look at the documentation (I'll build it as well)!
</p>
<p>
Here is the new doc test, demonstrating that cached_in_parent works with categories:
</p>
<pre class="wiki">sage: cython_code = ["from sage.structure.parent cimport Parent",
... "from sage.structure.element cimport Element",
... "from sage.all import Category, cached_in_parent_method",
... "cdef class MyElement(Element):",
... " cdef object x",
... " def __init__(self,P,x):",
... " self.x=x",
... " Element.__init__(self,P)",
... " def _repr_(self):",
... " return '<%s>'%self.x",
... " def __neg__(self):",
... " return MyElement(self.parent(),-self.x)",
... " def __hash__(self):",
... " return hash(self.x)",
... " def __cmp__(self, other):",
... " return cmp(self.x, (<MyElement>other).x)",
... "cdef class MyParent(Parent): pass",
... "class MyCategory(Category):",
... " def super_categories(self):",
... " return [Objects()]",
... " class ElementMethods:",
... " @cached_in_parent_method",
... " def test_cache(self):",
... " return -self"]
sage: cython('\n'.join(cython_code))
sage: C = MyCategory()
sage: P = MyParent(category=C)
sage: e1 = MyElement(P,5)
sage: e2 = MyElement(P,5)
sage: e3 = MyElement(P,6)
</pre><p>
We verify that attribute assignment does not work:
</p>
<pre class="wiki">sage: e1.bla = 1
Traceback (most recent call last):
...
AttributeError: '...MyElement' object has no attribute 'bla'
sage: P.bla = 1
Traceback (most recent call last):
...
AttributeError: '...MyParent' object has no attribute 'bla'
</pre><p>
Nevertheless, the cached method works, and it returns identical
output for equal elements, as expected:
</p>
<pre class="wiki">sage: e1.test_cache()
<-5>
sage: e1 is e2
False
sage: e1 == e2
True
sage: e2.test_cache() is e1.test_cache()
True
sage: e1 == e3
False
sage: e2.test_cache() == e3.test_cache()
False
</pre><p>
Back to the discussion about attributes: It is due to the <code>__cached_methods</code> attribute of <code>P</code> that the cache does not break in the example. It is not needed that the elements have that attribute.
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>
</p>
TicketSimonKingThu, 28 Apr 2011 09:27:31 GMTattachment set
https://trac.sagemath.org/ticket/11115
https://trac.sagemath.org/ticket/11115
<ul>
<li><strong>attachment</strong>
set to <em>trac11115-cached_in_parent_with_category.patch</em>
</li>
</ul>
<p>
Allow the use of cached_in_parent methods in the category framework
</p>
TicketnovoseltThu, 28 Apr 2011 14:33:12 GMT
https://trac.sagemath.org/ticket/11115#comment:28
https://trac.sagemath.org/ticket/11115#comment:28
<p>
I think that the current behaviour of the patch regarding cache storage is just fine. Getting more memory tends to be much easier and cheaper than getting a faster CPU.
</p>
<p>
I was avoiding using <code>@cached_method</code> before due to its issues with speed and documentation, but have started using it recently and it is so convenient! The more general, convenient, and well-behaved these decorators are, the more developers will adapt them.
</p>
<p>
While in principle it is not too much of a hassle to insert an extra attribute into your new class, it is quite annoying to always remember that if you decide to put a decorator on some method you have also to check if there is that extra attribute already defined or not. And as I understand, things don't break if you forget to do it, they just work slower than they should.
</p>
TicketSimonKingFri, 29 Apr 2011 07:04:55 GMT
https://trac.sagemath.org/ticket/11115#comment:29
https://trac.sagemath.org/ticket/11115#comment:29
<p>
Hi Andrey,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:28" title="Comment 28">novoselt</a>:
</p>
<blockquote class="citation">
<p>
And as I understand, things don't break if you forget to do it, they just work slower than they should.
</p>
</blockquote>
<p>
What scenario does your statement refer to? (1) The status quo, (2) the patch as it is, or (3) the patch without <code>sage.structure.element.Element.__cached_methods</code>?
</p>
<p>
Here is my view on these three:
</p>
<p>
(1) Cached methods have been improved in <a class="closed ticket" href="https://trac.sagemath.org/ticket/8611" title="enhancement: speed up cached_function and cached_method (closed: fixed)">#8611</a>, but they are still not fast enough, for my taste. Time and memory is wasted by assigning a <code>__doc__</code> string to each instance of a cached method (that is noticeable in the startup time). It can only be used in Python code. If a parent or an element does not accept attribute assignment then it can inherit a cached_method from the category -- but the cache breaks <em>and</em> there is a huge overhead. cached_in_parent_method can not be inherited from the category at all.
</p>
<p>
(2) cached_method can compete with a hand-written cache in Python, speed-wise, provided that attribute assignment is possible. <code>__doc__</code> is now replaced by <code>_sage_doc_</code>, which is used by sage's introspection anyway and computes the docstring only when requested. It can, to some extent, be used in Cython code. If a parent or an element does not accept attribute assignment then it can inherit a cached_method from the category: The return values <em>are</em> cached ("things don't break), but there is an overhead (though much less than in (1)). The price to pay is 0.24% more memory consumption at startup. cached_in_parent_method can be inherited from the category.
</p>
<p>
(3) (Hypothetical) Cached methods are supported for elements only if either the element allows attribute assignment, or the user did not forget to provide it with a public attribute <code>__cached_methods</code>. If neither of these conditions hold then things behave as in (1), for elements. In particular, for an element that does not satisfy the conditions, the cache of cached_method will break and will have a huge overhead. However, cached_in_parent_method is fully supported (that is since the cache is in the parent, and parents <em>have</em> <code>__cached_methods</code> in scenario (3)).
</p>
TicketSimonKingFri, 29 Apr 2011 07:50:19 GMT
https://trac.sagemath.org/ticket/11115#comment:30
https://trac.sagemath.org/ticket/11115#comment:30
<p>
PS: Here is another test.
</p>
<p>
With sage-4.7.alpha5 and no patches applied:
</p>
<pre class="wiki">sage: get_memory_usage()
839.96484375
sage: K = GF(next_prime(10^6))
sage: %time L = [K(i) for i in xrange(10^6)]
CPU times: user 4.00 s, sys: 0.06 s, total: 4.05 s
Wall time: 4.06 s
sage: get_memory_usage()
929.83203125
sage: 929.83203125 - 839.96484375
89.8671875000000
</pre><p>
With sage-4.7.alpha5 and the patches applied:
</p>
<pre class="wiki">sage: get_memory_usage()
841.9921875
sage: K = GF(next_prime(10^6))
sage: %time L = [K(i) for i in xrange(10^6)]
CPU times: user 4.25 s, sys: 0.03 s, total: 4.28 s
Wall time: 4.28 s
sage: get_memory_usage()
938.6953125
sage: 938.6953125 - 841.9921875
96.7031250000000
</pre><p>
I don't know why it became slower - as I mentioned above, the element creation in <code>GF(101)</code> became faster. However, when many elements are created then the memory consumption increases by something like 7.5%:
</p>
<pre class="wiki">sage: (96.7031250000000-89.8671875000000)/89.8671875000000
0.0760671129270625
</pre><p>
That may be too much.
</p>
TicketSimonKingFri, 29 Apr 2011 08:22:08 GMT
https://trac.sagemath.org/ticket/11115#comment:31
https://trac.sagemath.org/ticket/11115#comment:31
<p>
Apparently the different tendency of the test comes from choosing a different field size. If I return to <code>GF(101)</code>, I get
</p>
<p>
sage-4.7.alpha5 without patches:
</p>
<pre class="wiki">sage: get_memory_usage()
839.96484375
sage: K = GF(101)
sage: %time L = [K(i) for i in xrange(10^6)]
CPU times: user 2.75 s, sys: 0.00 s, total: 2.76 s
Wall time: 2.76 s
sage: get_memory_usage()
849.00390625
sage: 849.00390625-839.96484375
9.03906250000000
</pre><p>
With the patches:
</p>
<pre class="wiki">sage: get_memory_usage()
841.9921875
sage: K = GF(101)
sage: %time L = [K(i) for i in xrange(10^6)]
CPU times: user 2.66 s, sys: 0.00 s, total: 2.66 s
Wall time: 2.66 s
sage: get_memory_usage()
851.03125
sage: 851.03125-841.9921875
9.03906250000000
</pre><p>
So, the memory consumption in that scenario does not increase at all, and there is a little speedup.
</p>
<p>
I wonder why that is different with <code>GF(next_prime(10^6))</code>?
</p>
TicketnovoseltFri, 29 Apr 2011 12:38:15 GMT
https://trac.sagemath.org/ticket/11115#comment:32
https://trac.sagemath.org/ticket/11115#comment:32
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:29" title="Comment 29">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Hi Andrey,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:28" title="Comment 28">novoselt</a>:
</p>
<blockquote class="citation">
<p>
And as I understand, things don't break if you forget to do it, they just work slower than they should.
</p>
</blockquote>
<p>
What scenario does your statement refer to? (1) The status quo, (2) the patch as it is, or (3) the patch without <code>sage.structure.element.Element.__cached_methods</code>?
</p>
</blockquote>
<p>
Hi Simon, I was referring to (3) here, although I probably phrased it ambiguously. By "don't break" I meant that, as I understand, Sage will work and produce correct results if a user forgets to add a special caching attribute. But the cache <em>will</em> break and the speed will be lower. So I think that (2) is a better option.
</p>
TicketnovoseltFri, 29 Apr 2011 12:43:58 GMT
https://trac.sagemath.org/ticket/11115#comment:33
https://trac.sagemath.org/ticket/11115#comment:33
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:31" title="Comment 31">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
I wonder why that is different with <code>GF(next_prime(10^6))</code>?
</p>
</blockquote>
<p>
If I am not mistaken, creation of elements for "small" and "big" fields is somewhat different, something like all elements are created and stored somewhere for small fields, but for big ones they are created one-by-one.
</p>
<p>
Personally, I think that even 10% is fine in memory increase if it leads to an efficient and uniform caching...
</p>
TicketnbruinSat, 30 Apr 2011 21:42:02 GMT
https://trac.sagemath.org/ticket/11115#comment:34
https://trac.sagemath.org/ticket/11115#comment:34
<p>
Responding to <a class="ext-link" href="http://groups.google.com/group/sage-devel/msg/0aa5aca883603f64"><span class="icon"></span>http://groups.google.com/group/sage-devel/msg/0aa5aca883603f64</a>
</p>
<p>
Most machines have a limited resource (CPU cache) which means that in general, one expects that applications with a smaller memory footprint should have a tendency to run a bit faster. Given that I would have a slight preference towards option (3), i.e., keep elements small.
</p>
<p>
These things are notoriously difficult to quantify and are very processor dependent - good implementations of linear algebra libraries have to be tuned to be cache-friendly on each architecture.
</p>
<p>
The general principle is the following: General memory access is relatively slow, so CPUs have a cache (nowadays several levels) of fast memory so that repeated access to the same memory locations doesn't have to go all the way to main memory and can be done faster.
</p>
<p>
This works because memory usage tends to be temporally and spatially local. (temporal: the same memory locations tend to be accessed frequently before moving on; spatial: programs tend to access memory locations that are close together).
</p>
<p>
Making elements bigger will reduce both: If a CPU has enough cache to store N words and an element occupies s words, then a program will run quickly if its inner loop refers to at most N/s elements. As soon as it refers to more, the cache starts thrashing and you'll notice a poorer performance. As you see, small s is better. Your proposal would change s to s+1 ...
</p>
<p>
The spatial component comes in because caching rarely happens with a resolution of single words (because the overhead of storing which memory locations are cached). That's why it's good to have related data stored in consecutive memory. Increasing s also reduces that ... (apart from the fact that Python does not give us control over location of allocation anyway, but we can hope Python does this sanely).
</p>
<p>
Again, this is notoriously hard to quantify, but if you fix the problem and the architecture one can usually recognise drops in performance as data size increases, due to the different levels of cache starting to thrash. So, in general, small elements are better, both for memory use and for performance.
</p>
TicketnbruinSun, 01 May 2011 19:24:55 GMT
https://trac.sagemath.org/ticket/11115#comment:35
https://trac.sagemath.org/ticket/11115#comment:35
<p>
Just to make sure that the CPU cache effect is real, I tried a little example
</p>
<pre class="wiki">%cython
cpdef test(int nelts,int spacing,int repetitions):
cdef int i,j,r
cdef int *L
cdef int k
L=<int*>malloc(sizeof(int)*nelts*spacing)
for r in xrange(repetitions):
k=0
for j in xrange(nelts):
L[k]=L[k]+1
k += spacing
free(L)
</pre><p>
The following program times incrementing memory cells <code>2^30</code> times, while varying the number of cells (nelts) and their spacing in memory (spacing).
</p>
<pre class="wiki">for j in xrange(0,9):
for i in xrange(1,29-j):
tstart=os.times()[0]
test(2**i,2**j,2**(30-i))
tstop=os.times()[0]
print [i,j,30-i,tstop-tstart]
</pre><p>
The results are striking. Execution times vary by more than a factor 20 and one can see very definite thresholds where the execution time jumps. An excerpt from the output:
</p>
<pre class="wiki">[1, 0, 29, 1.8100000000000001]
[2, 0, 28, 1.5999999999999996]
[3, 0, 27, 1.4800000000000004]
[4, 0, 26, 1.7299999999999995]
[5, 0, 25, 1.5400000000000009]
[6, 0, 24, 1.4599999999999991]
[7, 0, 23, 1.4100000000000001]
[8, 0, 22, 1.3800000000000008]
[9, 0, 21, 1.3699999999999992]
...
[16, 0, 14, 1.3399999999999999]
[17, 0, 13, 1.3399999999999999]
[18, 0, 12, 2.1700000000000017]
[19, 0, 11, 2.6799999999999997]
[20, 0, 10, 2.7299999999999969]
[21, 0, 9, 2.740000000000002]
...
[28, 0, 2, 2.8500000000000014]
[9, 1, 21, 1.3599999999999994]
...
[16, 1, 14, 1.3499999999999943]
[17, 1, 13, 2.9500000000000028]
[18, 1, 12, 5.1700000000000017]
[19, 1, 11, 5.2199999999999989]
[12, 2, 18, 1.3599999999999852]
[13, 2, 17, 1.539999999999992]
[14, 2, 16, 1.5500000000000114]
[15, 2, 15, 1.539999999999992]
[16, 2, 14, 4.8200000000000216]
[17, 2, 13, 9.2099999999999795]
[11, 3, 19, 1.3500000000000227]
[12, 3, 18, 3.2199999999999704]
[13, 3, 17, 3.2200000000000273]
[14, 3, 16, 3.2199999999999704]
[15, 3, 15, 8.7700000000000387]
[16, 3, 14, 17.019999999999982]
[10, 4, 20, 1.3700000000000045]
[11, 4, 19, 6.1399999999999864]
[12, 4, 18, 6.1400000000000432]
[13, 4, 17, 6.1499999999999773]
[14, 4, 16, 15.779999999999973]
[15, 4, 15, 32.220000000000027]
[16, 4, 14, 33.379999999999995]
[17, 4, 13, 33.879999999999995]
[18, 4, 12, 34.210000000000036]
[19, 4, 11, 34.439999999999941]
[9, 5, 21, 1.3799999999999955]
[10, 5, 20, 5.1399999999999864]
[11, 5, 19, 5.1599999999999682]
[12, 5, 18, 5.0099999999999909]
[13, 5, 17, 14.540000000000077]
[14, 5, 16, 31.549999999999955]
[15, 5, 15, 32.299999999999955]
[7, 6, 23, 1.3999999999998636]
[8, 6, 22, 1.4100000000000819]
[9, 6, 21, 5.3800000000001091]
[10, 6, 20, 5.3899999999998727]
[11, 6, 19, 5.3900000000001]
[12, 6, 18, 14.480000000000018]
[13, 6, 17, 31.399999999999864]
[6, 7, 24, 1.4399999999998272]
[7, 7, 23, 1.4800000000000182]
[8, 7, 22, 5.4300000000000637]
[9, 7, 21, 5.4100000000000819]
[10, 7, 20, 5.3999999999998636]
[11, 7, 19, 14.960000000000036]
[12, 7, 18, 31.470000000000027]
[13, 7, 17, 33.029999999999973]
[14, 7, 16, 33.600000000000136]
[5, 8, 25, 1.5399999999999636]
[6, 8, 24, 1.6100000000001273]
[7, 8, 23, 6.0899999999999181]
[8, 8, 22, 5.8699999999998909]
[9, 8, 21, 5.7300000000000182]
[10, 8, 20, 22.1700000000003]
[11, 8, 19, 41.649999999999636]
[12, 8, 18, 64.5300000000002]
[13, 8, 17, 69.190000000000055]
[14, 8, 16, 73.199999999999818]
[15, 8, 15, 74.820000000000164]
[16, 8, 14, 69.440000000000055]
[17, 8, 13, 69.579999999999927]
[18, 8, 12, 70.349999999999909]
[19, 8, 11, 69.730000000000018]
[20, 8, 10, 77.869999999999891]
</pre>
TicketSimonKingSun, 01 May 2011 19:54:36 GMT
https://trac.sagemath.org/ticket/11115#comment:36
https://trac.sagemath.org/ticket/11115#comment:36
<p>
Hi Nils,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:35" title="Comment 35">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Just to make sure that the CPU cache effect is real, I tried a little example
</p>
</blockquote>
<p>
Thank you for your clear explanation! I am currently testing some variations of my patch: Both for parents and for elements, one may or may not have <code>__cached_methods</code> - if one has, then it can always inherit cached methods from the category framework. And if one has the attribute, then one may or may not search it in the <code>__getattr__</code> method - if one does then the speed penalty for missing attribute assignment is reduced.
</p>
<p>
I can already confirm that the timings are affected by the presence of the attribute. But I first need to finish my tests.
</p>
TicketSimonKingSun, 01 May 2011 21:25:01 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:37
https://trac.sagemath.org/ticket/11115#comment:37
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
My tests seem to indicate that (at least on my CPU) <code>__cached_methods</code> for parents does not hurt, but <code>__cached_methods</code> for elements can yield a slow-down. Changes in the <code>__getattr__</code> methods of parents or elements did not yield a significant slow-down.
</p>
<p>
So, I think it is a good idea to have <code>__cached_methods</code> for parents. It makes me wonder whether such attribute could serve an additional purpose: If the method resolution order does not find an attribute of a parent then it is searched in the category, by <code>getattr_from_other_class</code>. That takes a lot of time. So, shouldn't there be a shortpath so that looking up the same attribute becomes faster the second time?
</p>
<p>
That's to say: If a parent <code>P</code> inherits any attribute <code>foo</code> from the category then I suggest that <code>P.foo</code> should be stored in <code>P.__cached_methods["foo"]</code>. <code>__getattr__</code> needs to search <code>P.__cached_methods</code> anyway, and a lookup in a dictionary should be faster than calling <code>getattr_from_other_class</code>.
</p>
<p>
Concerning elements, it meanwhile seems better to not have <code>__cached_methods</code> by default. But it should be possible that it is provided by the user, so that cached methods inherited from the category can be stored there. The <code>__getattr__</code> of elements will test whether there is <code>__cached_methods</code> and search there.
</p>
<p>
So far, my tests indicate that the above approach is fine, speed-wise and memory-wise. My test is:
</p>
<pre class="wiki">a = get_memory_usage()
K = GF(2)
%time L = [K(i) for i in xrange(10^7)]
get_memory_usage() - a
a = get_memory_usage()
K = GF(101)
%time L = [K(i) for i in xrange(10^7)]
get_memory_usage() - a
a = get_memory_usage()
K = GF(next_prime(10^6))
%time L = [K(i) for i in xrange(10^7)]
get_memory_usage() - a
</pre><p>
So, it is formed by three scenarios. I first give the user CPU time and the memory consumption for sage-4.7.alpha5 plus the patches from <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>:
</p>
<pre class="wiki">25.28 s, 78.49609375
25.11 s, 0.0 # so, GF(101) needs the same amount of memory than for GF(2)
88.20 s, 794.9375
</pre><p>
Here are the same data for the current patches (i.e., with <code>__cached_methods</code> for both parents and elements):
</p>
<pre class="wiki">25.73 s, 78.49609375
25.11 s, 0.0
92.89 s, 864.21484375
</pre><p>
And here are the same data for a (not yet posted) patch that implements the ideas sketched above:
</p>
<pre class="wiki">25.33 s, 78.49609375
24.49 s, 0.0
87.13 s, 794.93359375
</pre><p>
I'll first catch some sleep, then I will add documentation about the use of <code>__cached_methods</code>, and of course I also need to run doctests.
</p>
TicketSimonKingMon, 02 May 2011 07:03:03 GMT
https://trac.sagemath.org/ticket/11115#comment:38
https://trac.sagemath.org/ticket/11115#comment:38
<p>
I just noticed that the additional attribute for parents could actually serve a third purpose: Providing lazy attributes for parent extension classes!
</p>
<p>
Namely, lazy attributes rely on the possibility to assign attributes. At least in the case of parent classes, if attribute assignment is impossible, they could do the next best thing and store stuff in <code>__cached_methods</code>!
</p>
TicketSimonKingMon, 02 May 2011 09:52:04 GMT
https://trac.sagemath.org/ticket/11115#comment:39
https://trac.sagemath.org/ticket/11115#comment:39
<p>
At the moment, it seems to me that the length of doc strings of cached_in_parent_method has a considerable influence (namely almost 4%) on the computation time, in some tests that actually do not involve a cached_in_parent_method. Frustrating. Can that really be? Should that be a reason to avoid documentation???
</p>
TicketSimonKingMon, 02 May 2011 11:10:40 GMT
https://trac.sagemath.org/ticket/11115#comment:40
https://trac.sagemath.org/ticket/11115#comment:40
<p>
Before I can submit a new patch, I must remove redundant tests from the documentation. It seems that if I make the documentation a little bit too long then the element creation tests become slower by 4%.
</p>
TicketSimonKingTue, 03 May 2011 07:29:18 GMTstatus, description changed; dependencies set
https://trac.sagemath.org/ticket/11115#comment:41
https://trac.sagemath.org/ticket/11115#comment:41
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>dependencies</strong>
set to <em>#9976</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=41">diff</a>)
</li>
</ul>
<p>
I have replaced the previous patches by a single patch. I am sorry in advance for the long post below.
</p>
<p>
It was indicated in previous posts that there was a certain danger of slowing down object creation. Also, one may think of startuptime. It seems to me that it is now solved.
</p>
<p>
I announced that I would like to use the new <code>__cached_methods</code> attribute of parents to store attributes that are obtained by slow attribute lookup in the category framework, and to make lazy attributes work on all parents.
</p>
<p>
Here are examples. I compare sage-4.7.alpha5 plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a> (referred to by "without patch") with sage-4.7.alpha5 plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a> plus the patch from here (referred to by "with patch").
</p>
<p>
<span class="underline">1. Startuptime</span>
With patch:
</p>
<pre class="wiki">...
== Slowest (including children) ==
1.155 sage.all (None)
0.275 sage.schemes.all (sage.all)
0.232 sage.misc.all (sage.all)
0.158 elliptic_curves.all (sage.schemes.all)
0.155 ell_rational_field (elliptic_curves.all)
...
</pre><p>
Without patch:
</p>
<pre class="wiki">...
== Slowest (including children) ==
1.229 sage.all (None)
0.278 sage.schemes.all (sage.all)
0.241 sage.misc.all (sage.all)
0.161 elliptic_curves.all (sage.schemes.all)
0.159 ell_rational_field (elliptic_curves.all)
...
</pre><p>
=> no slow down.
</p>
<p>
<span class="underline">2. Element creation</span>
</p>
<p>
This is with patch. I indicate the corresponding timings without patch in the comments:
</p>
<pre class="wiki">sage: a = get_memory_usage()
sage: K = GF(2)
sage: %time L = [K(i) for i in xrange(10^7)]
CPU times: user 25.23 s, sys: 0.02 s, total: 25.25 s
Wall time: 25.26 s # without patch: 25.52 s
sage: get_memory_usage() - a
78.49609375 # same as without patch
sage: a = get_memory_usage()
sage: K = GF(101)
sage: %time L = [K(i) for i in xrange(10^7)]
CPU times: user 25.13 s, sys: 0.03 s, total: 25.16 s
Wall time: 25.16 s # without patch: 26.20 s
sage: get_memory_usage() - a
0.0
sage: a = get_memory_usage()
sage: K = GF(next_prime(10^6))
sage: %time L = [K(i) for i in xrange(10^7)]
CPU times: user 87.40 s, sys: 0.24 s, total: 87.63 s
Wall time: 87.64 s # without patch: 88.87 s
sage: get_memory_usage() - a
794.94140625 # without patch: 794.94921875
</pre><p>
=> no slow down.
</p>
<p>
<span class="underline">3. Cached methods in Python code</span>
</p>
<p>
Here, I consider existing code, namely <code>gens()</code> and <code>groebner_basis()</code> for multivariate polynomial ideals. Again, the timings are with patch, the old timings are in comments
</p>
<pre class="wiki">sage: P.<x,y> = QQ[]
sage: I = P*[x,y]
sage: timeit('I.gens()',number=10^7)
10000000 loops, best of 3: 142 ns per loop
# Without patch:
# 10000000 loops, best of 3: 703 ns per loop
sage: timeit('I.groebner_basis()',number=10^7)
10000000 loops, best of 3: 249 ns per loop
# Without patch:
# 10000000 loops, best of 3: 746 ns per loop
</pre><p>
<span class="underline">4. Accessing parent attributes that are inherited from the category, if the element class of the category does not appear in the method resolution order</span>
</p>
<p>
It may happen that a parent is not an instance of the parent class of its category. That can be for two reasons: Either the category is not properly initialised (it is partially taken care of in <a class="closed ticket" href="https://trac.sagemath.org/ticket/9944" title="defect: categories for polynomial rings (closed: fixed)">#9944</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>), or it is a Cython class (if I remember correctly, they don't play well with dynamic classes). Then, attribute lookup must go beyond the usual method resolution order, and that's very slow. So, I suggest to use a shortpath in that case:
</p>
<pre class="wiki">sage: P.<x,y> = QQ[]
sage: P._is_category_initialized()
False
sage: P._init_category_(Algebras(QQ))
sage: P.sum
<bound method Algebras.parent_class.sum of Multivariate Polynomial Ring in x, y over Rational Field>
sage: P.sum.__module__
'sage.categories.commutative_additive_monoids'
sage: timeit('P.sum', number=10^6)
1000000 loops, best of 3: 755 ns per loop
# Without patch
# 1000000 loops, best of 3: 3.76 µs per loop
sage: P.__cached_methods['sum']
<bound method Algebras.parent_class.sum of Multivariate Polynomial Ring in x, y over Rational Field>
</pre><p>
<span class="underline">5. Cython: Lazy attributes and cached methods inherit from the category</span>
</p>
<p>
The following would not work at all without the patch.
</p>
<p>
We define a category (written in Cython) with some cached methods.
</p>
<pre class="wiki">sage: cython_code = ["from sage.all import cached_method, cached_in_parent_method, Category",
... "class MyCategory(Category):",
... " @cached_method",
... " def super_categories(self):",
... " return [Objects()]",
... " class ElementMethods:",
... " @cached_method",
... " def element_cache_test(self):",
... " return -self",
... " @cached_in_parent_method",
... " def element_via_parent_test(self):",
... " return -self",
... " class ParentMethods:",
... " @cached_method",
... " def one(self):",
... " return self.element_class(self,1)",
... " @cached_method",
... " def invert(self, x):",
... " return -x"]
sage: cython('\n'.join(cython_code))
</pre><p>
<em>Case 1</em>
</p>
<p>
Define elements and parents as Python classes (but in Cython code), so that attribute assignment is possible. That's the easy case.
</p>
<pre class="wiki">sage: cython_code = ["from sage.structure.element cimport Element",
... "class MyElement(Element):",
... " def __init__(self,P,x):",
... " self.x=x",
... " Element.__init__(self,P)",
... " def __neg__(self):",
... " return MyElement(self.parent(),-self.x)",
... " def _repr_(self):",
... " return '<%s>'%self.x",
... " def __hash__(self):",
... " return hash(self.x)",
... " def raw_test(self):",
... " return -self",
... "from sage.structure.parent cimport Parent",
... "class MyParent(Parent):",
... " Element = MyElement"]
sage: cython('\n'.join(cython_code))
sage: C = MyCategory()
sage: P = MyParent(category=C)
sage: e = MyElement(P,5)
</pre><p>
Cached method for the parent:
</p>
<pre class="wiki"># Cache without arguments
sage: P.one()
<1>
sage: P.one() is P.one()
True
sage: timeit('P.one()', number=10^6)
1000000 loops, best of 3: 210 ns per loop
# Cache with arguments
sage: P.invert(e)
<-5>
sage: P.invert(e) is P.invert(e)
True
sage: timeit('P.invert(e)', number=10^6)
1000000 loops, best of 3: 815 ns per loop
</pre><p>
Cached methods for elements (one with cache in the element, the other with cache in the parent):
</p>
<pre class="wiki"># Cache without arguments
sage: e.element_cache_test()
<-5>
sage: e.element_cache_test() is e.element_cache_test()
True
sage: timeit('e.element_cache_test()', number=10^6)
1000000 loops, best of 3: 173 ns per loop
# Cached in parent
sage: e.element_via_parent_test()
<-5>
sage: e.element_via_parent_test() is e.element_via_parent_test()
True
sage: timeit('e.element_via_parent_test()', number=10^6)
1000000 loops, best of 3: 574 ns per loop
# Comparison with element creation:
sage: e.raw_test()
<-5>
sage: e.raw_test() is e.raw_test()
False
sage: timeit('e.raw_test()', number=10^6)
1000000 loops, best of 3: 1.57 µs per loop
</pre><p>
<em>Case 2</em>
</p>
<p>
We use Cython classes for which attribute assignment is impossible. This is no problem at all, for the parent class. For the element class, we need to provide an additional public attribute <code>__cached_methods</code>, or things partially break:
</p>
<pre class="wiki">sage: cython_code = ["from sage.structure.element cimport Element",
... "cdef class MyBrokenElement(Element):",
... " cdef object x",
... " def __init__(self,P,x):",
... " self.x=x",
... " Element.__init__(self,P)",
... " def __neg__(self):",
... " return MyElement(self.parent(),-self.x)",
... " def _repr_(self):",
... " return '<%s>'%self.x",
... " def __hash__(self):",
... " return hash(self.x)",
... " def raw_test(self):",
... " return -self",
... "cdef class MyElement(Element):",
... " cdef public dict __cached_methods",
... " cdef object x",
... " def __init__(self,P,x):",
... " self.x=x",
... " Element.__init__(self,P)",
... " def __neg__(self):",
... " return MyElement(self.parent(),-self.x)",
... " def _repr_(self):",
... " return '<%s>'%self.x",
... " def __hash__(self):",
... " return hash(self.x)",
... " def raw_test(self):",
... " return -self",
... "from sage.structure.parent cimport Parent",
... "cdef class MyParent(Parent):",
... " Element = MyElement"]
sage: cython('\n'.join(cython_code))
sage: C = MyCategory()
sage: P = MyParent(category=C)
sage: ebroken = MyBrokenElement(P,5)
sage: e = MyElement(P,5)
</pre><p>
Every parent should have a <strong>lazy attribute</strong> <code>element_class</code> -- but so far, it used to fail on extension classes. That problem is solved by the patch:
</p>
<pre class="wiki">sage: P.element_class
<type '_mnt_local_king__sage_temp_mpc622_29604_tmp_2_spyx_0.MyElement'>
</pre><p>
If attribute assignment is impossible then the cache (for parents) still works, but gets a bit slower. Without the patch, the cache would break, and even a working cache would be slower.
</p>
<pre class="wiki">sage: P.one()
<1>
sage: P.one() is P.one()
True
sage: timeit('P.one()', number=10^6)
1000000 loops, best of 3: 685 ns per loop
# Cache with arguments
sage: P.invert(e)
<-5>
sage: P.invert(e) is P.invert(e)
True
sage: timeit('P.invert(e)', number=10^6)
1000000 loops, best of 3: 1.06 µs per loop
</pre><p>
We now consider the broken cache for elements. It turns out that the cached method for the "broken" element class can still be called, but is not cached. However, the cached-in-parent method <em>is</em> cached, but is terribly slow:
</p>
<pre class="wiki">sage: ebroken.element_cache_test()
<-5>
# This is broken:
sage: ebroken.element_cache_test() is ebroken.element_cache_test()
False
sage: ebroken.element_via_parent_test()
<-5>
# This is not broken:
sage: ebroken.element_via_parent_test() is ebroken.element_via_parent_test()
True
# But it is very very slow:
sage: timeit('ebroken.element_via_parent_test()')
625 loops, best of 3: 31.2 µs per loop
</pre><p>
However, the simple addition of a public dict <code>__cached_methods</code> make the cache work nicely (even though attribute assignment is still faster):
</p>
<pre class="wiki">sage: e.element_cache_test()
<-5>
sage: e.element_cache_test() is e.element_cache_test()
True
sage: timeit('e.element_cache_test()', number=10^6)
1000000 loops, best of 3: 921 ns per loop
# Cached in parent
sage: e.element_via_parent_test()
<-5>
sage: e.element_via_parent_test() is e.element_via_parent_test()
True
sage: timeit('e.element_via_parent_test()', number=10^6)
1000000 loops, best of 3: 1.13 µs per loop
# Comparison with element creation
sage: timeit('e.raw_test()', number=10^6)
1000000 loops, best of 3: 696 ns per loop
</pre><p>
<span class="underline">6. Clear cache on pickle</span>
</p>
<p>
There was a special class <code>ClearCacheOnPickle</code> in the cachfunc module. However, it was not sufficiently documented (and not provided with tests), and in fact it was broken. I made it work, and this is one is taken from the doc strings:
</p>
<pre class="wiki"> In the following example, we create a Python class that inherits
from multivariate polynomial ideals, but does not pickle cached
values. We provide the definition in Cython, however, since
interactive Cython definitions provide introspection by trac
ticket #9976, whereas Python definitions don't.
::
sage: P.<a,b,c,d> = QQ[]
sage: I = P*[a,b]
sage: classdef = ['from sage.misc.cachefunc import ClearCacheOnPickle',
... 'from sage.all import QQ',
... 'P = QQ["a","b","c","d"]; I = P*[P.gen(0),P.gen(1)]',
... 'class MyClass(ClearCacheOnPickle,I.__class__):',
... ' def __init__(self,ring,gens):',
... ' I.__class__.__init__(self,ring,gens)',
... ' def __getnewargs__(self):',
... ' return (self._Ideal_generic__ring,self._Ideal_generic__gens)']
sage: cython('\n'.join(classdef))
We destroy the cache of two methods of ``I`` on purpose
(demonstrating that the two different implementations of cached
methods are correctly dealt with). Pickling ``I`` preserves the
cache::
sage: I.gens.set_cache('bar')
sage: I.groebner_basis.set_cache('foo',algorithm='singular')
sage: J = loads(dumps(I))
sage: J.gens()
'bar'
sage: J.groebner_basis(algorithm='singular')
'foo'
However, if we have an ideal that additionally descends from
:class:`ClearCacheOnPickle`, the carefully corrupted cache is not
pickled::
sage: A = MyClass(P,[a,b])
sage: A
Ideal (a, b) of Multivariate Polynomial Ring in a, b, c, d over Rational Field
sage: A.gens.set_cache('foo')
sage: A.groebner_basis.set_cache('bar',algorithm='singular')
sage: A.gens()
'foo'
sage: A.groebner_basis(algorithm='singular')
'bar'
sage: B = loads(dumps(A))
sage: B.gens()
[a, b]
sage: B.groebner_basis(algorithm='singular')
[a, b]
sage: A.gens()
'foo'
</pre><p>
Doctests pass for me. So, I guess it is in need of review, again!
</p>
<p>
For the patchbot:
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>
</p>
<p>
Apply trac11115-cached_cython.patch
</p>
TicketSimonKingThu, 05 May 2011 14:22:02 GMT
https://trac.sagemath.org/ticket/11115#comment:42
https://trac.sagemath.org/ticket/11115#comment:42
<p>
There is another introspection ticket, namely <a class="closed ticket" href="https://trac.sagemath.org/ticket/11298" title="defect: Extend the capabilities of Sage's introspection (closed: fixed)">#11298</a>, that should probably proceed this ticket. It does some changes in sage.rings.polynomial.mult_polynomial_ideal, and therefore some line numbers in the introspection tests in sage.misc.cachefunc needed to be modified.
</p>
<p>
It is taken care of by the most recent patch version.
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11298" title="defect: Extend the capabilities of Sage's introspection (closed: fixed)">#11298</a>
</p>
<p>
Apply trac11115-cached_cython.patch
</p>
TicketSimonKingThu, 05 May 2011 14:27:20 GMTdescription changed
https://trac.sagemath.org/ticket/11115#comment:43
https://trac.sagemath.org/ticket/11115#comment:43
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=43">diff</a>)
</li>
</ul>
TicketSimonKingThu, 05 May 2011 14:27:32 GMTdependencies changed
https://trac.sagemath.org/ticket/11115#comment:44
https://trac.sagemath.org/ticket/11115#comment:44
<ul>
<li><strong>dependencies</strong>
changed from <em>#9976</em> to <em>#9976, #11298</em>
</li>
</ul>
TicketSimonKingThu, 05 May 2011 18:36:56 GMT
https://trac.sagemath.org/ticket/11115#comment:45
https://trac.sagemath.org/ticket/11115#comment:45
<p>
FWIW, the long tests pass for me. I really don't know what the patchbot is complaining about.
</p>
TicketrobertwbWed, 11 May 2011 05:55:40 GMTdependencies changed
https://trac.sagemath.org/ticket/11115#comment:46
https://trac.sagemath.org/ticket/11115#comment:46
<ul>
<li><strong>dependencies</strong>
changed from <em>#9976, #11298</em> to <em>sage-4.7, #9976, #11298</em>
</li>
</ul>
TicketSimonKingSat, 14 May 2011 13:43:38 GMT
https://trac.sagemath.org/ticket/11115#comment:47
https://trac.sagemath.org/ticket/11115#comment:47
<p>
After investing some work on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9944" title="defect: categories for polynomial rings (closed: fixed)">#9944</a>, I am now resuming work on cached methods.
</p>
<p>
Let's discuss elements again. The options were:
</p>
<ol><li>Have <code>__cached_methods</code> as a cdef public attribute for <code>sage.structure.element.Element</code>. 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.
</li></ol><ol start="2"><li>Do not have <code>__cached_method</code> for elements, but let <code>__getattr__</code> 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 <code>__cached_methods</code>. Disadvantage: The <code>__getattr__</code> method of elements needs to look for <code>__cached_methods</code>, which yields another slowdown.
</li></ol><p>
I therefore suggest a third solution:
</p>
<ol start="3"><li>Keep <code>sage.structure.element.Element</code> untouched (both the memory footprint and the <code>__getattr__</code> method). Instead, introduce a new class <code>sage.structure.element.ElementWithCache</code>: It derives from <code>Element</code> and only adds a <code>__cached_method</code> attribute and a modified <code>__getattr__</code>. Hence, if the user wants an element class with the possibility to inherit cached methods or lazy attributes from the category then either
</li></ol><blockquote>
<blockquote>
<p>
i) the class must allow attribute assignment (<code>class MyElement(Element)</code> will do), or
</p>
</blockquote>
</blockquote>
<blockquote>
<blockquote>
<p>
ii) the class must be derived from <code>ElementWithCache</code> instead of <code>Element</code> (<code>cdef class MyElement(ElementWithCache)</code> will do).
</p>
</blockquote>
</blockquote>
<p>
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.
</p>
TicketSimonKingSat, 14 May 2011 20:50:29 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/11115#comment:48
https://trac.sagemath.org/ticket/11115#comment:48
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>Cacheable subclass of Element; radical application of cached_methods</em>
</li>
</ul>
<p>
I am now preparing a new patch that will implement my suggestion from the previous comment. Moreover, I'll radically apply cached methods.
</p>
<p>
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 <em>any</em> method that has only <code>self</code> as an argument and will not be modified by side-effects of other methods. <strong>Any.</strong>
</p>
<p>
The reason for that project is the fact that even a simple-most possible method, such as
</p>
<pre class="wiki">def is_exact(self):
return True
</pre><p>
takes about 690 ns on my machine, and
</p>
<pre class="wiki">def prec(self):
return self._prec
</pre><p>
even takes about 2.1 µs.
</p>
<p>
The cached versions are much faster,
</p>
<pre class="wiki">@cached_method
def is_exact(self):
return True
@cached_method
def prec(self):
return self._prec
</pre><p>
only takes about 375 ns on the same machine, with my patch.
</p>
<p>
Thus I expect to obtain a general speedup of arithmetics. The question is whether there will be a noticeable increase of memory consumption.
</p>
TicketSimonKingMon, 16 May 2011 06:46:53 GMTstatus, description changed; work_issues deleted
https://trac.sagemath.org/ticket/11115#comment:49
https://trac.sagemath.org/ticket/11115#comment:49
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=49">diff</a>)
</li>
<li><strong>work_issues</strong>
<em>Cacheable subclass of Element; radical application of cached_methods</em> deleted
</li>
</ul>
<p>
I updated <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11115/trac11115-cached_cython.patch" title="Attachment 'trac11115-cached_cython.patch' in Ticket #11115">trac11115-cached_cython.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11115/trac11115-cached_cython.patch" title="Download"></a>: 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 "...".
</p>
<p>
And I posted a new patch <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11115/trac11115-ElementWithCache.patch" title="Attachment 'trac11115-ElementWithCache.patch' in Ticket #11115">trac11115-ElementWithCache.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11115/trac11115-ElementWithCache.patch" title="Download"></a>. 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.
</p>
<p>
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 <code>sage.structure.element.ElementWithCachedMethod</code>. I doubt that it will be frequently used, but at least it is there.
</p>
<p>
I guess adding @cached_method on front of any small method of rings and algebras shall be on a different ticket.
</p>
<p>
Back to "needs review"!
</p>
TicketSimonKingMon, 16 May 2011 06:47:52 GMT
https://trac.sagemath.org/ticket/11115#comment:50
https://trac.sagemath.org/ticket/11115#comment:50
<p>
I forgot to inform the patchbot:
</p>
<p>
Depends on sage-4.7, <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11298" title="defect: Extend the capabilities of Sage's introspection (closed: fixed)">#11298</a>
</p>
<p>
Apply trac11115-cached_cython.patch trac11115-<a class="missing wiki">ElementWithCache?</a>.patch
</p>
TicketSimonKingMon, 23 May 2011 13:31:11 GMTdependencies, description changed
https://trac.sagemath.org/ticket/11115#comment:51
https://trac.sagemath.org/ticket/11115#comment:51
<ul>
<li><strong>dependencies</strong>
changed from <em>sage-4.7, #9976, #11298</em> to <em>sage-4.7, #9976, #11298, 11342</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=51">diff</a>)
</li>
</ul>
<p>
I am currently working on different tickets. One of them, namely <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a>, changes the getattr method of parents and elements. Since <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> is more basic than this ticket, it should be applied first. Hence, I introduced it as a new dependency and updated both patches accordingly.
</p>
<p>
At least for me, all doc tests pass.
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9976" title="enhancement: Decorated functions/methods have generic signature in documentation (closed: fixed)">#9976</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11298" title="defect: Extend the capabilities of Sage's introspection (closed: fixed)">#11298</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a>
</p>
<p>
Apply trac11115-cached_cython.patch trac11115-<a class="missing wiki">ElementWithCache?</a>.patch
</p>
TicketSimonKingWed, 25 May 2011 06:49:47 GMTdependencies changed
https://trac.sagemath.org/ticket/11115#comment:52
https://trac.sagemath.org/ticket/11115#comment:52
<ul>
<li><strong>dependencies</strong>
changed from <em>sage-4.7, #9976, #11298, 11342</em> to <em>sage-4.7, #9976, #11298, #11342</em>
</li>
</ul>
TicketSimonKingThu, 26 May 2011 07:16:51 GMT
https://trac.sagemath.org/ticket/11115#comment:53
https://trac.sagemath.org/ticket/11115#comment:53
<p>
I had to rebase the first patch because of a change in <a class="closed ticket" href="https://trac.sagemath.org/ticket/11298" title="defect: Extend the capabilities of Sage's introspection (closed: fixed)">#11298</a>.
</p>
TicketSimonKingThu, 26 May 2011 13:17:19 GMTattachment set
https://trac.sagemath.org/ticket/11115
https://trac.sagemath.org/ticket/11115
<ul>
<li><strong>attachment</strong>
set to <em>trac11115-ElementWithCache.patch</em>
</li>
</ul>
<p>
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
</p>
TicketSimonKingThu, 26 May 2011 13:18:22 GMT
https://trac.sagemath.org/ticket/11115#comment:54
https://trac.sagemath.org/ticket/11115#comment:54
<p>
I have just updated the second patch, because I had some misprints in the documentation of the new class <code>sage.structure.element.ElementWithCachedMethod</code>.
</p>
TicketvbraunMon, 22 Aug 2011 21:55:35 GMT
https://trac.sagemath.org/ticket/11115#comment:55
https://trac.sagemath.org/ticket/11115#comment:55
<p>
<code>trac11115-cached_cython.patch</code> applies with fuzz 2 but seems ok. Please rediff anyways ;-)
</p>
TicketSimonKingMon, 22 Aug 2011 22:24:10 GMT
https://trac.sagemath.org/ticket/11115#comment:56
https://trac.sagemath.org/ticket/11115#comment:56
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:55" title="Comment 55">vbraun</a>:
</p>
<blockquote class="citation">
<p>
<code>trac11115-cached_cython.patch</code> applies with fuzz 2
</p>
</blockquote>
<p>
Really? I am now using sage-4.7.2.alpha2, and both patches (of course applied after the two patches from <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a>) match fine:
</p>
<pre class="wiki">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 http://trac.sagemath.org/sage_trac/raw-attachment/ticket/11115/trac11115-cached_cython.patch
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 http://trac.sagemath.org/sage_trac/raw-attachment/ticket/11115/trac11115-ElementWithCache.patch
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
</pre>
TicketvbraunMon, 22 Aug 2011 22:38:19 GMT
https://trac.sagemath.org/ticket/11115#comment:57
https://trac.sagemath.org/ticket/11115#comment:57
<p>
I just noticed that the fuzz is because I applied your <code>trac9138-categories_for_rings.patch</code> first. I guess we don't consider that as a prerequisite for this ticket or for <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> any more.
</p>
TicketSimonKingTue, 23 Aug 2011 07:33:06 GMT
https://trac.sagemath.org/ticket/11115#comment:58
https://trac.sagemath.org/ticket/11115#comment:58
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:57" title="Comment 57">vbraun</a>:
</p>
<blockquote class="citation">
<p>
I just noticed that the fuzz is because I applied your <code>trac9138-categories_for_rings.patch</code> first. I guess we don't consider that as a prerequisite for this ticket or for <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> any more.
</p>
</blockquote>
<p>
Yes, the dependency is opposite. First <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, then <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>.
</p>
TicketSimonKingTue, 23 Aug 2011 08:03:38 GMT
https://trac.sagemath.org/ticket/11115#comment:59
https://trac.sagemath.org/ticket/11115#comment:59
<p>
With sage-4.7.2.alpha2 plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> and this ticket, I get a rather strange doctest error:
</p>
<pre class="wiki">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]
</pre><p>
Hence, 0.0 becomes -0.0.
</p>
<p>
Worse is another error:
</p>
<pre class="wiki">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]
**********************************************************************
</pre>
TicketSimonKingTue, 23 Aug 2011 08:23:47 GMT
https://trac.sagemath.org/ticket/11115#comment:60
https://trac.sagemath.org/ticket/11115#comment:60
<p>
I just found that <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> is not to blame. Both failures occur when only <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> is applied.
</p>
TicketvbraunTue, 23 Aug 2011 08:28:44 GMT
https://trac.sagemath.org/ticket/11115#comment:61
https://trac.sagemath.org/ticket/11115#comment:61
<p>
Thats x86 FPU for you! Working as intended!
</p>
TicketSimonKingTue, 23 Aug 2011 08:30:32 GMT
https://trac.sagemath.org/ticket/11115#comment:62
https://trac.sagemath.org/ticket/11115#comment:62
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:61" title="Comment 61">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Thats x86 FPU for you! Working as intended!
</p>
</blockquote>
<p>
What does that mean?
</p>
TicketSimonKingTue, 23 Aug 2011 08:34:34 GMT
https://trac.sagemath.org/ticket/11115#comment:63
https://trac.sagemath.org/ticket/11115#comment:63
<p>
Arrgh. Both errors occur in UNPATCHED sage-4.7.2.alpha2.
</p>
TicketvbraunTue, 23 Aug 2011 08:48:43 GMT
https://trac.sagemath.org/ticket/11115#comment:64
https://trac.sagemath.org/ticket/11115#comment:64
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:62" title="Comment 62">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:61" title="Comment 61">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Thats x86 FPU for you! Working as intended!
</p>
</blockquote>
<p>
What does that mean?
</p>
</blockquote>
<p>
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...
</p>
TicketSimonKingTue, 23 Aug 2011 17:28:36 GMT
https://trac.sagemath.org/ticket/11115#comment:65
https://trac.sagemath.org/ticket/11115#comment:65
<p>
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.
</p>
TicketSimonKingSun, 11 Sep 2011 17:12:52 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/11115#comment:66
https://trac.sagemath.org/ticket/11115#comment:66
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>Pickling of cached functions</em>
</li>
</ul>
<p>
I'd like to add one functionality: Pickling of cached functions!
</p>
<p>
Cached methods can be pickled. However, cached functions can not. Without my patch (in sage-4.6.2):
</p>
<pre class="wiki">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
</pre><p>
With my patch:
</p>
<pre class="wiki">sage: loads(dumps(cunningham_prime_factors))
Cached version of None
</pre><p>
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.
</p>
TicketSimonKingSun, 11 Sep 2011 17:40:39 GMTstatus, description changed
https://trac.sagemath.org/ticket/11115#comment:67
https://trac.sagemath.org/ticket/11115#comment:67
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=67">diff</a>)
</li>
</ul>
<p>
The problem was easy to solve: The new <code>__reduce__</code> 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.
</p>
<p>
Example:
</p>
<pre class="wiki">sage: type(cunningham_prime_factors)
<type 'sage.misc.cachefunc.CachedFunction'>
sage: loads(dumps(cunningham_prime_factors)) is cunningham_prime_factors #indirect doctest
True
</pre><p>
Apply trac11115-cached_cython.patch trac11115-<a class="missing wiki">ElementWithCache?</a>.patch trac11115_cached_function_pickling.patch
</p>
TicketleifMon, 12 Sep 2011 13:23:24 GMTdependencies changed
https://trac.sagemath.org/ticket/11115#comment:68
https://trac.sagemath.org/ticket/11115#comment:68
<ul>
<li><strong>dependencies</strong>
changed from <em>sage-4.7, #9976, #11298, #11342</em> to <em>#9976 #11298 #11342</em>
</li>
</ul>
TicketSimonKingTue, 13 Sep 2011 09:13:53 GMT
https://trac.sagemath.org/ticket/11115#comment:69
https://trac.sagemath.org/ticket/11115#comment:69
<p>
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?
</p>
<p>
Apply <code>trac11115-cached_cython.patch</code> <code>trac11115-ElementWithCache.patch</code> <code>trac11115_cached_function_pickling.patch</code>
</p>
TicketSimonKingTue, 13 Sep 2011 09:15:27 GMTattachment set
https://trac.sagemath.org/ticket/11115
https://trac.sagemath.org/ticket/11115
<ul>
<li><strong>attachment</strong>
set to <em>trac11115_element_with_cache.patch</em>
</li>
</ul>
<p>
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
</p>
TicketSimonKingTue, 13 Sep 2011 09:18:14 GMTdescription changed
https://trac.sagemath.org/ticket/11115#comment:70
https://trac.sagemath.org/ticket/11115#comment:70
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=70">diff</a>)
</li>
</ul>
<p>
Escaping the names did not work. So, I renamed one patch.
</p>
<p>
Apply trac11115-cached_cython.patch trac11115_element_with_cache.patch trac11115_cached_function_pickling.patch
</p>
TicketleifFri, 16 Sep 2011 17:04:15 GMTreviewer set
https://trac.sagemath.org/ticket/11115#comment:71
https://trac.sagemath.org/ticket/11115#comment:71
<ul>
<li><strong>reviewer</strong>
set to <em>Nicolas M. Thiéry, Andrey Novoseltsev</em>
</li>
</ul>
<p>
Ping, to the (or potential) reviewers... (<a class="closed ticket" href="https://trac.sagemath.org/ticket/11068" title="enhancement: Basic implementation of one- and twosided ideals of non-commutative ... (closed: fixed)">#11068</a>, which already has positive review, depends on this one.)
</p>
<p>
Simon, perhaps re-advertise it on sage-devel.
</p>
TicketSimonKingFri, 16 Sep 2011 17:34:51 GMT
https://trac.sagemath.org/ticket/11115#comment:72
https://trac.sagemath.org/ticket/11115#comment:72
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:71" title="Comment 71">leif</a>:
</p>
<blockquote class="citation">
<p>
Simon, perhaps re-advertise it on sage-devel.
</p>
</blockquote>
<p>
AGAIN?? Would be the fourth or fifth time...
</p>
TicketleifFri, 16 Sep 2011 20:19:36 GMT
https://trac.sagemath.org/ticket/11115#comment:73
https://trac.sagemath.org/ticket/11115#comment:73
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:72" title="Comment 72">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:71" title="Comment 71">leif</a>:
</p>
<blockquote class="citation">
<p>
Simon, perhaps re-advertise it on sage-devel.
</p>
</blockquote>
<p>
AGAIN?? Would be the fourth or fifth time...
</p>
</blockquote>
<p>
Oh, then perhaps just dig the last thread, hinting at that it still needs review...
</p>
TicketvbraunSat, 17 Sep 2011 20:40:51 GMT
https://trac.sagemath.org/ticket/11115#comment:74
https://trac.sagemath.org/ticket/11115#comment:74
<p>
I'm getting fuzz with sage-4.7.2.alpha2 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a>. Is there a dependency missing or does it just need to be rediffed?
</p>
TicketvbraunSat, 17 Sep 2011 20:43:39 GMT
https://trac.sagemath.org/ticket/11115#comment:75
https://trac.sagemath.org/ticket/11115#comment:75
<p>
Sorry, the fuzz comes from a conflict with <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>. 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?
</p>
TicketSimonKingSat, 17 Sep 2011 20:59:17 GMTstatus, work_issues changed
https://trac.sagemath.org/ticket/11115#comment:76
https://trac.sagemath.org/ticket/11115#comment:76
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
changed from <em>Pickling of cached functions</em> to <em>Rebase wrt #9138</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:75" title="Comment 75">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Sorry, the fuzz comes from a conflict with <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>. 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?
</p>
</blockquote>
<p>
Good idea.
</p>
TicketSimonKingSat, 17 Sep 2011 21:09:43 GMTstatus, dependencies, description changed; work_issues deleted
https://trac.sagemath.org/ticket/11115#comment:77
https://trac.sagemath.org/ticket/11115#comment:77
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#9976 #11298 #11342</em> to <em>#9976 #11298 #11342 #9138</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=77">diff</a>)
</li>
<li><strong>work_issues</strong>
<em>Rebase wrt #9138</em> deleted
</li>
</ul>
<p>
Rebasing is done!
</p>
<p>
For the patchbot:
</p>
<p>
Apply trac11115-cached_cython.patch trac11115_element_with_cache.patch trac11115_cached_function_pickling.patch
</p>
TicketvbraunSat, 17 Sep 2011 21:19:31 GMT
https://trac.sagemath.org/ticket/11115#comment:78
https://trac.sagemath.org/ticket/11115#comment:78
<p>
I still get the fuzz on top of sage-4.7.2.alpha2 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>, can you double check that you got the right patch?
</p>
TicketvbraunSat, 17 Sep 2011 21:34:49 GMT
https://trac.sagemath.org/ticket/11115#comment:79
https://trac.sagemath.org/ticket/11115#comment:79
<p>
Hmm, I get a different patch if I download at a remote machine. Seems like a caching problem. Didn't you have a similar problem a while ago? Maybe the trac server announces that it is ok to cache the patch files by proxies?
</p>
TicketleifSat, 17 Sep 2011 23:04:19 GMT
https://trac.sagemath.org/ticket/11115#comment:80
https://trac.sagemath.org/ticket/11115#comment:80
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:79" title="Comment 79">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Hmm, I get a different patch if I download at a remote machine. Seems like a caching problem. Didn't you have a similar problem a while ago? Maybe the trac server announces that it is ok to cache the patch files by proxies?
</p>
</blockquote>
<p>
<code>wget -N --no-cache ...</code>
</p>
<p>
You can put default options into <code>~/.wgetrc</code>.
</p>
TicketSimonKingSat, 17 Sep 2011 23:07:28 GMT
https://trac.sagemath.org/ticket/11115#comment:81
https://trac.sagemath.org/ticket/11115#comment:81
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:79" title="Comment 79">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Hmm, I get a different patch if I download at a remote machine. Seems like a caching problem. Didn't you have a similar problem a while ago?
</p>
</blockquote>
<p>
Yes. My sysadmin told me how to switch of the cache proxy on my machine. If I remember correctly, it was similar to what Leif suggests.
</p>
<blockquote class="citation">
<p>
Maybe the trac server announces that it is ok to cache the patch files by proxies?
</p>
</blockquote>
<p>
I don't know if it is trac's fault.
</p>
<p>
Anyway, the patchbot finds that there is one failure:
</p>
<pre class="wiki">File "/levi/scratch/robertwb/buildbot/sage-4.7.1/devel/sage-11115/sage/misc/cachefunc.pyx", line 228:
sage: print sage_getdoc(O.wrapped_method)
Expected:
File: ...
(starting at line ...) some doc for a wrapped cython method
Got:
some doc for a wrapped cython method
<BLANKLINE>
</pre><p>
That's strange, because <code>sage_getdoc</code> is supposed to also provide the file and position - and it usually does. Do you get that error as well? I don't (I just verified it)!
</p>
TicketvbraunSun, 18 Sep 2011 16:29:10 GMT
https://trac.sagemath.org/ticket/11115#comment:82
https://trac.sagemath.org/ticket/11115#comment:82
<p>
I get the same error with sage-4.7.2.alpha2 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>. Are you also using the same starting point?
</p>
TicketSimonKingSun, 18 Sep 2011 17:31:52 GMT
https://trac.sagemath.org/ticket/11115#comment:83
https://trac.sagemath.org/ticket/11115#comment:83
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:82" title="Comment 82">vbraun</a>:
</p>
<blockquote class="citation">
<p>
I get the same error with sage-4.7.2.alpha2 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>. Are you also using the same starting point?
</p>
</blockquote>
<p>
Bad. The problem is that my patch queue starts with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, but it continues which some polybori, <code>M4RI</code> and <code>M4RIE</code> patches/spkgs, that are a bit tedious to get rid of. SO, I will need some time before I can test.
</p>
TicketSimonKingSun, 18 Sep 2011 17:40:35 GMT
https://trac.sagemath.org/ticket/11115#comment:84
https://trac.sagemath.org/ticket/11115#comment:84
<p>
Could you try with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11734" title="defect: sage_wraps should only read the sources of wrapped functions when needed. (closed: fixed)">#11734</a> in addition?
</p>
<p>
I found a sage-4.7.2.alpha1 on my machine, with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11431" title="defect: Conversion from Singular to Sage (closed: fixed)">#11431</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11734" title="defect: sage_wraps should only read the sources of wrapped functions when needed. (closed: fixed)">#11734</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11068" title="enhancement: Basic implementation of one- and twosided ideals of non-commutative ... (closed: fixed)">#11068</a> applied. The error does not occur in this setting. Since the error is related with documentation and with wrapped methods, I could imagine that <a class="closed ticket" href="https://trac.sagemath.org/ticket/11734" title="defect: sage_wraps should only read the sources of wrapped functions when needed. (closed: fixed)">#11734</a> is needed to get rid of the error.
</p>
<p>
Best reagrds,
Simon
</p>
TicketvbraunSun, 18 Sep 2011 18:17:08 GMT
https://trac.sagemath.org/ticket/11115#comment:85
https://trac.sagemath.org/ticket/11115#comment:85
<p>
No, still failing with the same error.
</p>
TicketSimonKingSun, 18 Sep 2011 18:27:36 GMT
https://trac.sagemath.org/ticket/11115#comment:86
https://trac.sagemath.org/ticket/11115#comment:86
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:85" title="Comment 85">vbraun</a>:
</p>
<blockquote class="citation">
<p>
No, still failing with the same error.
</p>
</blockquote>
<p>
That's amazing. Definitely <code>sage_getdoc</code> is supposed to provide the file and position information, not just the mere doc string.
</p>
<p>
Please try the following (I did this with unpatched sage-4.6.2):
</p>
<pre class="wiki">sage: cython("""
....: def test(x):
....: 'some doc string'
....: return None
....: """)
sage: from sage.misc.sageinspect import sage_getdoc
sage: print sage_getdoc(test)
File: _mnt_local_king__sage_temp_mpc622_14846_tmp_0_spyx_0.pyx
(starting at line 7) some doc string
</pre><p>
Do you really get nothing more than <code>'some doc string'</code>?
</p>
TicketSimonKingSun, 18 Sep 2011 18:30:47 GMT
https://trac.sagemath.org/ticket/11115#comment:87
https://trac.sagemath.org/ticket/11115#comment:87
<p>
Aha! There may be a difference between the doctest and the example from my previous post.
</p>
<pre class="wiki">sage: cython("""
....: class Bar(object):
....: def _sage_doc_(self):
....: return 'some doc string'
....: """)
sage: sage_getdoc(Bar())
'some doc string\n'
</pre><p>
So, the file information <em>is</em> missing (both in sage-4.6.2 and sage-4.7.2.alpha2).
</p>
TicketSimonKingSun, 18 Sep 2011 18:43:22 GMT
https://trac.sagemath.org/ticket/11115#comment:88
https://trac.sagemath.org/ticket/11115#comment:88
<p>
Anyway. I will try to get a fresh sage-4.7.2.alpha2 and see if I get the same error.
</p>
TicketleifSun, 18 Sep 2011 19:44:27 GMT
https://trac.sagemath.org/ticket/11115#comment:89
https://trac.sagemath.org/ticket/11115#comment:89
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:88" title="Comment 88">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Anyway. I will try to get a fresh sage-4.7.2.alpha2 and see if I get the same error.
</p>
</blockquote>
<p>
You could also try the <a class="ext-link" href="http://boxen.math.washington.edu/home/leif/Sage/release/sage-4.7.2.alpha3-prerelease/sage-4.7.2.alpha3-prerelease.tar"><span class="icon"></span>current alpha3 prerelease</a>, which I think won't change much more, and already contains <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>. On top of almost all patches, I've applied <a class="closed ticket" href="https://trac.sagemath.org/ticket/11749" title="enhancement: Remove unneeded imports (closed: fixed)">#11749</a>, which makes changes to almost every file of the Sage library.
</p>
TicketSimonKingSun, 18 Sep 2011 21:06:35 GMT
https://trac.sagemath.org/ticket/11115#comment:90
https://trac.sagemath.org/ticket/11115#comment:90
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:89" title="Comment 89">leif</a>:
</p>
<blockquote class="citation">
<p>
You could also try the <a class="ext-link" href="http://boxen.math.washington.edu/home/leif/Sage/release/sage-4.7.2.alpha3-prerelease/sage-4.7.2.alpha3-prerelease.tar"><span class="icon"></span>current alpha3 prerelease</a>
</p>
</blockquote>
<p>
Too late, I already built sage-4.7.2.alpha2...
</p>
TicketSimonKingSun, 18 Sep 2011 21:08:41 GMT
https://trac.sagemath.org/ticket/11115#comment:91
https://trac.sagemath.org/ticket/11115#comment:91
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:90" title="Comment 90">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Too late, I already built sage-4.7.2.alpha2...
</p>
</blockquote>
<p>
Or perhaps I'll get alpha3 and do everything else tomorrow morning.
</p>
TicketSimonKingSun, 18 Sep 2011 23:02:10 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:92
https://trac.sagemath.org/ticket/11115#comment:92
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
I couldn't resist to test right now.
</p>
<p>
Unfortunately, I can <em>not</em> reproduce the problem. I get:
</p>
<pre class="wiki">king@mpc622:/mnt/local/king/SAGE/debug/sage-4.7.2.alpha3-prerelease/devel/sage$ ../../sage -t sage/misc/cachefunc.pyx
sage -t "devel/sage-main/sage/misc/cachefunc.pyx"
[15.4 s]
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 15.4 seconds
king@mpc622:/mnt/local/king/SAGE/debug/sage-4.7.2.alpha3-prerelease/devel/sage$ hg qapplied
trac11115-cached_cython.patch
trac11115_element_with_cache.patch
trac11115_cached_function_pickling.patch
</pre><p>
As you can see, it is sage-4.7.2.alpha3 with the three patches from here applied. So, is the error platform dependent? What could one do, then?
</p>
TicketSimonKingMon, 19 Sep 2011 08:54:24 GMT
https://trac.sagemath.org/ticket/11115#comment:93
https://trac.sagemath.org/ticket/11115#comment:93
<p>
Leif and/or Volker, what happends if you do the following? Do you get essentially the same as I? In particular, can the file be found?
</p>
<pre class="wiki">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):',
... ' wrapped_method = cached_method(test_meth,name="wrapped_method")']
sage: cython('\n'.join(cython_code))
sage: O = MyClass()
sage: from sage.misc.sageinspect import sage_getfile, sage_getargspec
sage: sage_getargspec(O.wrapped_method)
ArgSpec(args=['self', 'x'], varargs=None, keywords=None, defaults=None)
sage: sage_getfile(O.wrapped_method)
'/mnt/local/king/SAGE/debug/sage-4.7.2.alpha3-prerelease/devel/sage/_mnt_local_king__sage_temp_mpc622_4939_tmp_1_spyx_0.pyx'
</pre><p>
Does <code>O.wrapped_method?</code> and <code>O.wrapped_method??</code> work for you, or is it impossible to open the source file? If the latter is the case: What is <code>O.wrapped_method.__module__</code>?
</p>
<p>
I suppose that the problem is actually located in <code>_sage_getdoc_unformatted</code>, because for me it already returns the file and embedding information:
</p>
<pre class="wiki">from sage.misc.sageinspect import _sage_getdoc_unformatted
sage: _sage_getdoc_unformatted(O.wrapped_method)
'File: _mnt_local_king__sage_temp_mpc622_4939_tmp_1_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
</pre>
TicketSimonKingMon, 19 Sep 2011 09:03:53 GMT
https://trac.sagemath.org/ticket/11115#comment:94
https://trac.sagemath.org/ticket/11115#comment:94
<p>
PS:
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:93" title="Comment 93">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
sage: sage_getfile(O.wrapped_method)
'/mnt/local/king/SAGE/debug/sage-4.7.2.alpha3-prerelease/devel/sage/_mnt_local_king<span class="underline">sage_temp_mpc622_4939_tmp_1_spyx_0.pyx'
</span></p>
</blockquote>
<p>
It just occurs to me that this is the wrong location: Such file does not exist. A file of that name is only somewhere in <code>SAGE_TMP</code>, but not in <code>SAGE_ROOT</code>.
</p>
<p>
I don't know if it would solve the problem, though.
</p>
TicketSimonKingMon, 19 Sep 2011 09:28:15 GMT
https://trac.sagemath.org/ticket/11115#comment:95
https://trac.sagemath.org/ticket/11115#comment:95
<p>
The problem with the wrong source file directory reminded me another ticket, namely <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/11791" title="enhancement: Introspection for interactively defined classes with metaclass (needs_work)">#11791</a>.
</p>
<p>
When I start with sage-4.7.2.alpha3-prerelease and add <a class="closed ticket" href="https://trac.sagemath.org/ticket/11768" title="enhancement: Get source code for parent/element classes of categories (closed: fixed)">#11768</a>+<a class="needs_work ticket" href="https://trac.sagemath.org/ticket/11791" title="enhancement: Introspection for interactively defined classes with metaclass (needs_work)">#11791</a>+<a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, I get the following:
</p>
<pre class="wiki">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):',
....: ... ' wrapped_method = cached_method(test_meth,name="wrapped_method")']
sage: cython('\n'.join(cython_code))
sage: O = MyClass()
sage: from sage.misc.sageinspect import sage_getfile, sage_getdoc
sage: sage_getfile(O.wrapped_method)
'/mnt/local/king/.sage/temp/mpc622/8600//spyx/_mnt_local_king__sage_temp_mpc622_8600_tmp_0_spyx/_mnt_local_king__sage_temp_mpc622_8600_tmp_0_spyx_0.pyx'
sage: print sage_getdoc(O.wrapped_method)
File: _mnt_local_king__sage_temp_mpc622_8600_tmp_0_spyx_0.pyx
(starting at line 6) some doc for a wrapped cython method
</pre><p>
Note that on my machine I do not need <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/11791" title="enhancement: Introspection for interactively defined classes with metaclass (needs_work)">#11791</a> for avoiding the doctest error. But perhaps <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/11791" title="enhancement: Introspection for interactively defined classes with metaclass (needs_work)">#11791</a> solves the problem for you?
</p>
TicketvbraunMon, 19 Sep 2011 09:35:45 GMT
https://trac.sagemath.org/ticket/11115#comment:96
https://trac.sagemath.org/ticket/11115#comment:96
<p>
Mysteriously, the <code>_sage_getdoc_unformatted</code> function works for me:
</p>
<pre class="wiki">sage: sage: _sage_getdoc_unformatted(O.wrapped_method)
'File: _home_vbraun__sage_temp_volker_desktop_stp_dias_ie_17521_tmp_0_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
</pre><p>
I'm compiling pre-alpha-3 version right now to see if that helps.
</p>
<p>
Also, I get errors applying <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/11791" title="enhancement: Introspection for interactively defined classes with metaclass (needs_work)">#11791</a> on top of sage-4.7.2.alpha2 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11734" title="defect: sage_wraps should only read the sources of wrapped functions when needed. (closed: fixed)">#11734</a>.
</p>
TicketSimonKingMon, 19 Sep 2011 09:41:41 GMT
https://trac.sagemath.org/ticket/11115#comment:97
https://trac.sagemath.org/ticket/11115#comment:97
<p>
No, it is all totally different. It should have nothing to do with <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/11791" title="enhancement: Introspection for interactively defined classes with metaclass (needs_work)">#11791</a> (but of course you may try).
</p>
<p>
In the code in my example, one starts with a function <code>test_meth</code>, that has a doc string. When <code>test_meth</code> is turned into a cached method, the doc string of <code>test_meth</code> is <em>directly</em> used as a doc string for the cached method.
</p>
<p>
When I do
</p>
<pre class="wiki">sage: print test_meth.__doc__
File: _mnt_local_king__sage_temp_mpc622_8600_tmp_0_spyx_0.pyx (starting at line 6)
some doc for a wrapped cython method
</pre><p>
then the doc string contains the file info.
</p>
<p>
You do <em>not</em> get that file info, right? But then, it has nothing to do with cached methods, because <code>test_meth</code> is a plain cpdef Cython function.
</p>
<p>
What happens when you do <code>sage_getfile(test_meth)</code> or <code>test_meth??</code>?
</p>
TicketSimonKingMon, 19 Sep 2011 09:43:06 GMT
https://trac.sagemath.org/ticket/11115#comment:98
https://trac.sagemath.org/ticket/11115#comment:98
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:96" title="Comment 96">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Also, I get errors applying <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/11791" title="enhancement: Introspection for interactively defined classes with metaclass (needs_work)">#11791</a> on top of sage-4.7.2.alpha2 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11342" title="enhancement: Make getattr faster on parents and elements (closed: fixed)">#11342</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11734" title="defect: sage_wraps should only read the sources of wrapped functions when needed. (closed: fixed)">#11734</a>.
</p>
</blockquote>
<p>
When I start with alpha3 and then first add <a class="closed ticket" href="https://trac.sagemath.org/ticket/11768" title="enhancement: Get source code for parent/element classes of categories (closed: fixed)">#11768</a>, followed by <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/11791" title="enhancement: Introspection for interactively defined classes with metaclass (needs_work)">#11791</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, then everything applies without fuzz.
</p>
TicketSimonKingMon, 19 Sep 2011 09:57:45 GMT
https://trac.sagemath.org/ticket/11115#comment:99
https://trac.sagemath.org/ticket/11115#comment:99
<p>
Sorry for posting again. But could you test <code>print test_meth.__doc__</code> with any sage version on your machine?
</p>
TicketvbraunMon, 19 Sep 2011 10:03:20 GMT
https://trac.sagemath.org/ticket/11115#comment:100
https://trac.sagemath.org/ticket/11115#comment:100
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:97" title="Comment 97">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
What happens when you do <code>sage_getfile(test_meth)</code> or <code>test_meth??</code>?
</p>
</blockquote>
<pre class="wiki">sage: sage_getfile(test_meth)
'/home/vbraun/Sage/sage/devel/sage/_home_vbraun__sage_temp_volker_desktop_stp_dias_ie_5615_tmp_0_spyx_0.pyx'
sage: test_meth??
Type: builtin_function_or_method
Base Class: <type 'builtin_function_or_method'>
String Form: <built-in function test_meth>
Namespace: Interactive
Definition: test_meth(self, x)
Source:
cpdef test_meth(self,x):
"some doc for a wrapped cython method"
return -x
sage: test_meth.__doc__
'File: _home_vbraun__sage_temp_volker_desktop_stp_dias_ie_5615_tmp_0_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
</pre>
TicketSimonKingMon, 19 Sep 2011 10:13:45 GMT
https://trac.sagemath.org/ticket/11115#comment:101
https://trac.sagemath.org/ticket/11115#comment:101
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:100" title="Comment 100">vbraun</a>:
</p>
<blockquote class="citation">
<p>
sage: test_meth.<span class="underline">doc</span>
'File: _home_vbraun<span class="underline">sage_temp_volker_desktop_stp_dias_ie_5615_tmp_0_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
</span></p>
</blockquote>
<p>
That is extremely strange. If you look at the <code>_sage_doc_</code> method in <code>sage.misc.cachefunc</code> then you'll find:
</p>
<div class="wiki-code"><div class="code"><pre> f <span class="o">=</span> <span class="bp">self</span><span class="o">.</span>f
<span class="k">if</span> <span class="nb">hasattr</span><span class="p">(</span>f<span class="p">,</span> <span class="s">"func_doc"</span><span class="p">):</span>
<span class="k">try</span><span class="p">:</span>
sourcelines <span class="o">=</span> sage_getsourcelines<span class="p">(</span>f<span class="p">)</span>
<span class="k">except</span> <span class="ne">IOError</span><span class="p">:</span>
sourcelines <span class="o">=</span> <span class="bp">None</span>
<span class="k">if</span> sourcelines <span class="ow">is</span> <span class="ow">not</span> <span class="bp">None</span><span class="p">:</span>
<span class="kn">import</span> <span class="nn">os</span>
SAGE_ROOT <span class="o">=</span> os<span class="o">.</span>environ<span class="p">[</span><span class="s">'SAGE_ROOT'</span><span class="p">]</span>
filename <span class="o">=</span> sage_getfile<span class="p">(</span>f<span class="p">)</span>
<span class="c"># The following is a heuristics to get</span>
<span class="c"># the file name of the cached function</span>
<span class="c"># or method</span>
<span class="k">if</span> filename<span class="o">.</span>startswith<span class="p">(</span>SAGE_ROOT<span class="o">+</span><span class="s">'/devel/sage/'</span><span class="p">):</span>
filename <span class="o">=</span> filename<span class="p">[</span><span class="nb">len</span><span class="p">(</span>SAGE_ROOT<span class="o">+</span><span class="s">'/devel/sage/'</span><span class="p">):]</span>
<span class="k">elif</span> <span class="s">'site-packages/'</span> <span class="ow">in</span> filename<span class="p">:</span>
filename <span class="o">=</span> filename<span class="o">.</span>split<span class="p">(</span><span class="s">'site-packages/'</span><span class="p">,</span><span class="mi">1</span><span class="p">)[</span><span class="mi">1</span><span class="p">]</span>
file_info <span class="o">=</span> <span class="s">"File: </span><span class="si">%s</span><span class="s"> (starting at line </span><span class="si">%d</span><span class="s">)"</span><span class="o">%</span><span class="p">(</span>filename<span class="p">,</span>sourcelines<span class="p">[</span><span class="mi">1</span><span class="p">])</span>
doc <span class="o">=</span> file_info<span class="o">+</span><span class="p">(</span>f<span class="o">.</span>func_doc <span class="ow">or</span> <span class="s">''</span><span class="p">)</span>
<span class="k">else</span><span class="p">:</span>
doc <span class="o">=</span> f<span class="o">.</span>func_doc
<span class="k">else</span><span class="p">:</span>
doc <span class="o">=</span> f<span class="o">.</span>__doc__
<span class="k">return</span> doc
</pre></div></div><p>
Here, <code>self</code> is <code>O.wrapped_method</code>, and thus <code>f</code> is <code>test_meth</code>. Since a Cython function has no attribute <code>func_doc</code>, the method <code>_sage_doc_</code> simply returns <code>f.__doc__</code> in our case, which <em>does</em> contain the file information.
</p>
<p>
Or is it the case that <code>test_meth</code> has a <code>func_doc</code> attribute on your machine?
</p>
TicketvbraunMon, 19 Sep 2011 10:43:27 GMT
https://trac.sagemath.org/ticket/11115#comment:102
https://trac.sagemath.org/ticket/11115#comment:102
<p>
After some prodding around, i noticed this:
</p>
<pre class="wiki">sage: from sage.misc.sageinspect import _extract_embedded_position
sage: import sage.misc.sagedoc
sage: r = 'File: _home_vbraun__sage_temp_volker_desktop_stp_dias_ie_17521_tmp_0_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
sage: sage.misc.sagedoc.format(r,embedded=False)
'File: _home_vbraun__sage_temp_volker_desktop_stp_dias_ie_17521_tmp_0_s\npyx_0.pyx (starting at line 6) some doc for a wrapped cython method\n'
sage: _extract_embedded_position(_)
(' some doc for a wrapped cython method\n', '/home/vbraun/Sage/sage/devel/sage/_home_vbraun__sage_temp_volker_desktop_stp_dias_ie_17521_tmp_0_s\npyx_0.pyx', 6)
</pre><p>
(is the <code>\n</code> in the middle of the name intentional?) vs.
</p>
<pre class="wiki">sage: r = 'File: _mnt_local_king__sage_temp_mpc622_4939_tmp_1_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
sage: sage.misc.sagedoc.format(r,embedded=False)
'File: _mnt_local_king__sage_temp_mpc622_4939_tmp_1_spyx_0.pyx\n(starting at line 6) some doc for a wrapped cython method\n'
sage: _extract_embedded_position(_)
sage:
</pre><p>
See the source of <code>sage_getdoc()</code>
</p>
TicketSimonKingMon, 19 Sep 2011 11:07:58 GMT
https://trac.sagemath.org/ticket/11115#comment:103
https://trac.sagemath.org/ticket/11115#comment:103
<p>
It is more and more strange. I don't know whether the line break in the name has a meaning or not. But for sure, <code>_extract_embedded_position(_)</code> does return something meaningful in your second example. I get:
</p>
<pre class="wiki">sage: r = 'File: _mnt_local_king__sage_temp_mpc622_4939_tmp_1_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
sage: print r
File: _mnt_local_king__sage_temp_mpc622_4939_tmp_1_spyx_0.pyx (starting at line 6)
some doc for a wrapped cython method
sage: print sage.misc.sagedoc.format(r,embedded=False)File: _mnt_local_king__sage_temp_mpc622_4939_tmp_1_spyx_0.pyx
(starting at line 6) some doc for a wrapped cython method
</pre><p>
But still I find it most puzzling that on your machine <code>O.wrapped_method._sage_doc_()</code> does not return the same as <code>O.wrapped_method.f.__doc__</code> - or does it?
</p>
TicketvbraunMon, 19 Sep 2011 11:11:11 GMT
https://trac.sagemath.org/ticket/11115#comment:104
https://trac.sagemath.org/ticket/11115#comment:104
<p>
They both return the same:
</p>
<pre class="wiki">sage: O.wrapped_method._sage_doc_()
'File: _home_vbraun__sage_temp_volker_desktop_stp_dias_ie_22122_tmp_0_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
sage: O.wrapped_method.f.__doc__
'File: _home_vbraun__sage_temp_volker_desktop_stp_dias_ie_22122_tmp_0_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
</pre><p>
But whenever <code>_extract_embedded_position</code> does not return <code>None</code> the line number is chopped off in <code>sage_doc()</code>.
</p>
TicketSimonKingMon, 19 Sep 2011 11:24:30 GMT
https://trac.sagemath.org/ticket/11115#comment:105
https://trac.sagemath.org/ticket/11115#comment:105
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:104" title="Comment 104">vbraun</a>:
</p>
<blockquote class="citation">
<p>
But whenever <code>_extract_embedded_position</code> does not return <code>None</code> the line number is chopped off in <code>sage_doc()</code>.
</p>
</blockquote>
<p>
I see, that could be it. However, as I have indicated above, <code>_extract_embedded_position</code> returns something non-None in both your examples, on my machine.
</p>
<p>
But let us consider the doc of <code>sage_getdoc</code>: It states that the embedded position is stripped from the doc string. Hence, actually the embedding should be shown with <code>_sage_getdoc_unformatted</code>, but not with <code>_sage_getdoc</code>.
</p>
<p>
So, after all, the fact that the embedded position is <em>not</em> stripped on my machine could be a bug.
</p>
TicketSimonKingMon, 19 Sep 2011 11:27:25 GMT
https://trac.sagemath.org/ticket/11115#comment:106
https://trac.sagemath.org/ticket/11115#comment:106
<p>
More precisely: There is a difference on my machine between <code>sage_getdoc</code> and <code>_sage_getdoc_unformatted</code>, but the difference on you machine is different...
</p>
<p>
I get
</p>
<pre class="wiki">sage: from sage.misc.sageinspect import sage_getdoc, _sage_getdoc_unformatted
sage: _sage_getdoc_unformatted(O.wrapped_method)
'File: _mnt_local_king__sage_temp_mpc622_9687_tmp_0_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
sage: sage_getdoc(O.wrapped_method)
'File: _mnt_local_king__sage_temp_mpc622_9687_tmp_0_spyx_0.pyx\n(starting at line 6) some doc for a wrapped cython method\n'
</pre><p>
whereas you seem to get just <code>'some doc for a wrapped cython method'</code> when calling <code>sage_doc</code>, right?
</p>
TicketSimonKingMon, 19 Sep 2011 11:34:10 GMT
https://trac.sagemath.org/ticket/11115#comment:107
https://trac.sagemath.org/ticket/11115#comment:107
<p>
I suggest to go through <code>sage_getdoc</code> step by step. For me, it is as follows:
</p>
<pre class="wiki">sage: from sage.misc.sageinspect import sage_getdoc, _sage_getdoc_unformatted, _extract_embedded_position
sage: sage.misc.sageinspect.EMBEDDED_MODE
False
sage: obj = O.wrapped_method
sage: r = _sage_getdoc_unformatted(obj)
sage: s = sage.misc.sagedoc.format(str(r), embedded=False)
sage: print s
File: _mnt_local_king__sage_temp_mpc622_9687_tmp_0_spyx_0.pyx
(starting at line 6) some doc for a wrapped cython method
sage: print r
File: _mnt_local_king__sage_temp_mpc622_9687_tmp_0_spyx_0.pyx (starting at line 6)
some doc for a wrapped cython method
</pre><p>
Is the result for s fundamentally different on your machine?
</p>
<pre class="wiki">sage: pos = _extract_embedded_position(s)
sage: pos is None
True
</pre><p>
Aha! And I suppose you get <code>pos!=None</code>!
</p>
TicketvbraunMon, 19 Sep 2011 11:37:42 GMT
https://trac.sagemath.org/ticket/11115#comment:108
https://trac.sagemath.org/ticket/11115#comment:108
<p>
Yes, the formatting runs sphinxify, which inserts newlines:
</p>
<pre class="wiki">sage: r = 'File: _mnt_local_king__sage_temp_mpc622_4939_tmp_1_spyx_0.pyx (starting at line 6)\nsome doc for a wrapped cython method'
sage: from sagenb.misc.sphinxify import sphinxify
sage: sphinxify(r, format='text')
'File: _mnt_local_king__sage_temp_mpc622_4939_tmp_1_spyx_0.pyx\n(starting at line 6) some doc for a wrapped cython method\n'
</pre><p>
You were lucky and the newline is right between the file name and the <code>(starting</code>. When that happens, the <code>sage.misc.sageinspect.__embedded_position_re</code> regular expression fails to match.
</p>
TicketSimonKingMon, 19 Sep 2011 11:42:39 GMT
https://trac.sagemath.org/ticket/11115#comment:109
https://trac.sagemath.org/ticket/11115#comment:109
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:108" title="Comment 108">vbraun</a>:
</p>
<blockquote class="citation">
<p>
You were lucky and the newline is right between the file name and the <code>(starting</code>. When that happens, the <code>sage.misc.sageinspect.__embedded_position_re</code> regular expression fails to match.
</p>
</blockquote>
<p>
The fact that the position of the inserted line breaks depends on the length of the file name seems pretty much to be a bug in <code>sage.misc.sagedoc.format</code>, don't you think?
</p>
TicketvbraunMon, 19 Sep 2011 11:45:05 GMT
https://trac.sagemath.org/ticket/11115#comment:110
https://trac.sagemath.org/ticket/11115#comment:110
<p>
Come to think of it, I was probably just unlucky. Sphinxify tries to put the newline in a space, but my filename is too long so it is forced to insert it mid-word. So the doctests breaks for be because my hostname is too long ;-)
</p>
<p>
The bug is in <code>sage_doc()</code>, it should first try to match regular expressions and only afterwards format the string (by inserting newlines at random places).
</p>
TicketSimonKingMon, 19 Sep 2011 11:56:08 GMT
https://trac.sagemath.org/ticket/11115#comment:111
https://trac.sagemath.org/ticket/11115#comment:111
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:110" title="Comment 110">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Come to think of it, I was probably just unlucky.
</p>
</blockquote>
<p>
The fact that one can be unlucky is a bug.
</p>
<blockquote class="citation">
<p>
The bug is in <code>sage_doc()</code>...
</p>
</blockquote>
<p>
Really? I'd rather locate it in <code>sage.misc.sagedoc.format</code>...
</p>
<blockquote class="citation">
<p>
, it should first try to match regular expressions and only afterwards format the string (by inserting newlines at random places).
</p>
</blockquote>
<p>
No. <code>format' should not apply the whole formatting business to the first line of the string, if that line contains an embedding information. Then, </code>sage_getdoc` would not be in trouble at all.
</p>
TicketSimonKingMon, 19 Sep 2011 12:16:04 GMT
https://trac.sagemath.org/ticket/11115#comment:112
https://trac.sagemath.org/ticket/11115#comment:112
<p>
I think, I can make it work with a change in <code>sage.misc.sagedoc.format</code>. By consequence, sage_getdoc will return the doc without embedding information (that's to say, in the same way as you find it), and _sage_getdoc_unformatted will always contain the embedding information.
</p>
<p>
Procedure: I'll open a new ticket for that problem, and make it a dependency.
</p>
TicketSimonKingMon, 19 Sep 2011 13:43:18 GMTdependencies changed; work_issues set
https://trac.sagemath.org/ticket/11115#comment:113
https://trac.sagemath.org/ticket/11115#comment:113
<ul>
<li><strong>dependencies</strong>
changed from <em>#9976 #11298 #11342 #9138</em> to <em>#9976 #11298 #11342 #9138 #11815</em>
</li>
<li><strong>work_issues</strong>
set to <em>rebase relative to #11815</em>
</li>
</ul>
TicketSimonKingMon, 19 Sep 2011 13:44:35 GMT
https://trac.sagemath.org/ticket/11115#comment:114
https://trac.sagemath.org/ticket/11115#comment:114
<p>
I added the dependency, but now I have to rebase it, because now some doc test will behave as they currently only do on Volker's machine...
</p>
TicketSimonKingMon, 19 Sep 2011 14:06:13 GMTattachment set
https://trac.sagemath.org/ticket/11115
https://trac.sagemath.org/ticket/11115
<ul>
<li><strong>attachment</strong>
set to <em>trac11115_cached_function_pickling.patch</em>
</li>
</ul>
<p>
Pickling of cached functions
</p>
TicketSimonKingMon, 19 Sep 2011 14:11:43 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:115
https://trac.sagemath.org/ticket/11115#comment:115
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
I modified the last of the three patches, so that the doctests of sage/misc/cachefunc.pyx pass.
</p>
<p>
By the new dependency <a class="closed ticket" href="https://trac.sagemath.org/ticket/11815" title="defect: Embedding information in doc strings must not be formatted (closed: fixed)">#11815</a>, sage_getdoc will now correctly strip the embedding information, even if one has a rather lengthy file name. Thus, needs review!
</p>
<p>
Apply trac11115-cached_cython.patch trac11115_element_with_cache.patch trac11115_cached_function_pickling.patch
</p>
TicketSimonKingMon, 19 Sep 2011 14:37:50 GMT
https://trac.sagemath.org/ticket/11115#comment:116
https://trac.sagemath.org/ticket/11115#comment:116
<p>
And the good news is that I can't really see any doc test error in the log of the patchbot...
</p>
TicketvbraunMon, 19 Sep 2011 17:00:31 GMT
https://trac.sagemath.org/ticket/11115#comment:117
https://trac.sagemath.org/ticket/11115#comment:117
<p>
How is one now supposed to find out whether a function without arguments is cached?
</p>
<pre class="wiki">sage: class Foo:
... def __init__(self):
... pass
... @cached_method
... def f(self):
... return self._x*z+y
...
sage: a = Foo()
sage: a.f.is_in_cache()
Traceback (most recent call last):
...
TypeError: 'NoneType' object is not callable
</pre><p>
I haven't added the <code>sage_getdoc()</code> fixes yet to my patch queue, but I think this problem is unrelated.
</p>
TicketSimonKingMon, 19 Sep 2011 17:30:19 GMTstatus, work_issues changed
https://trac.sagemath.org/ticket/11115#comment:118
https://trac.sagemath.org/ticket/11115#comment:118
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
changed from <em>rebase relative to #11815</em> to <em>is_in_cache without arguments</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:117" title="Comment 117">vbraun</a>:
</p>
<blockquote class="citation">
<p>
How is one now supposed to find out whether a function without arguments is cached?
</p>
</blockquote>
<p>
That's a bug. Fixing it now...
</p>
TicketSimonKingMon, 19 Sep 2011 17:35:50 GMTattachment set
https://trac.sagemath.org/ticket/11115
https://trac.sagemath.org/ticket/11115
<ul>
<li><strong>attachment</strong>
set to <em>trac11115-cached_cython.patch</em>
</li>
</ul>
<p>
Cythonized cached_method: Usable in Cython code, faster than handwritten Python cache. Special case for methods w/o arguments. Make <a class="missing wiki">ClearCacheOnPickle?</a> usable. Lazy attributes
</p>
TicketSimonKingMon, 19 Sep 2011 17:37:22 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/11115#comment:119
https://trac.sagemath.org/ticket/11115#comment:119
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>is_in_cache without arguments</em> deleted
</li>
</ul>
<p>
Done (changing the first of the three patches)
</p>
<p>
New doctest:
</p>
<pre class="wiki">
sage: P.<x,y> = QQ[]
sage: I = P*[x,y]
sage: I.gens.is_in_cache()
False
sage: I.gens()
[x, y]
sage: I.gens.is_in_cache()
True
</pre><p>
Apply trac11115-cached_cython.patch trac11115_element_with_cache.patch trac11115_cached_function_pickling.patch
</p>
TicketvbraunMon, 26 Sep 2011 22:14:47 GMTattachment set
https://trac.sagemath.org/ticket/11115
https://trac.sagemath.org/ticket/11115
<ul>
<li><strong>attachment</strong>
set to <em>trac_11115_reviewer.patch</em>
</li>
</ul>
<p>
Reviewer patch
</p>
TicketvbraunMon, 26 Sep 2011 22:15:56 GMTreviewer changed
https://trac.sagemath.org/ticket/11115#comment:120
https://trac.sagemath.org/ticket/11115#comment:120
<ul>
<li><strong>reviewer</strong>
changed from <em>Nicolas M. Thiéry, Andrey Novoseltsev</em> to <em>Nicolas M. Thiéry, Andrey Novoseltsev, Volker Braun</em>
</li>
</ul>
<p>
I've added a reviewer patch. Everything else is fine with me. Please set it to positive review if you agree with patch.
</p>
TicketSimonKingMon, 26 Sep 2011 23:00:45 GMTstatus, description changed
https://trac.sagemath.org/ticket/11115#comment:121
https://trac.sagemath.org/ticket/11115#comment:121
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=121">diff</a>)
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:120" title="Comment 120">vbraun</a>:
</p>
<blockquote class="citation">
<p>
I've added a reviewer patch. Everything else is fine with me. Please set it to positive review if you agree with patch.
</p>
</blockquote>
<p>
Thank you very much, both for the review and for your reviewer patch!
</p>
<p>
Apply trac11115-cached_cython.patch trac11115_element_with_cache.patch trac11115_cached_function_pickling.patch trac_11115_reviewer.patch
</p>
TicketjdemeyerTue, 04 Oct 2011 08:04:53 GMTmilestone changed
https://trac.sagemath.org/ticket/11115#comment:122
https://trac.sagemath.org/ticket/11115#comment:122
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.7.2</em> to <em>sage-4.7.3</em>
</li>
</ul>
TicketSimonKingFri, 07 Oct 2011 15:52:21 GMTdescription changed
https://trac.sagemath.org/ticket/11115#comment:123
https://trac.sagemath.org/ticket/11115#comment:123
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=123">diff</a>)
</li>
</ul>
TicketjdemeyerSun, 16 Oct 2011 15:45:18 GMTmilestone changed
https://trac.sagemath.org/ticket/11115#comment:124
https://trac.sagemath.org/ticket/11115#comment:124
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.7.3</em> to <em>sage-pending</em>
</li>
</ul>
TicketjdemeyerTue, 18 Oct 2011 20:29:30 GMTdescription changed
https://trac.sagemath.org/ticket/11115#comment:125
https://trac.sagemath.org/ticket/11115#comment:125
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=125">diff</a>)
</li>
</ul>
TicketjdemeyerSat, 22 Oct 2011 21:01:20 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/11115#comment:126
https://trac.sagemath.org/ticket/11115#comment:126
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>docbuild</em>
</li>
</ul>
<p>
This gives problems with the documentation:
</p>
<pre class="wiki">sphinx-build -b html -d /mnt/usb1/scratch/jdemeyer/merger/sage-4.7.3.alpha1/devel/sage/doc/output/doctrees/en/reference -A hide_pdf_link
s=1 /mnt/usb1/scratch/jdemeyer/merger/sage-4.7.3.alpha1/devel/sage/doc/en/reference /mnt/usb1/scratch/jdemeyer/merger/sage-4.7.3.alpha1/de
vel/sage/doc/output/html/en/reference
Running Sphinx v1.0.4
loading pickled environment... not yet created
building [html]: targets for 933 source files that are out of date
updating environment: 933 added, 0 changed, 0 removed
reading sources... [ 0%] algebras
reading sources... [ 0%] arithgroup
[...]
reading sources... [ 99%] structure
reading sources... [ 99%] tensor
reading sources... [100%] todolist
<autodoc>:0: (ERROR/3) Unexpected indentation.
<autodoc>:0: (ERROR/3) Unexpected indentation.
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [ 0%] algebras
writing output... [ 0%] arithgroup
[...]
writing output... [ 99%] tensor
writing output... [100%] todolist
writing additional files... genindex py-modindex search
copying images... [ 16%] media/homology/simplices.png
copying images... [ 33%] media/homology/rp2.png
copying images... [ 50%] media/heawood-graph-latex.png
copying images... [ 66%] media/homology/torus.png
copying images... [ 83%] media/homology/torus_labelled.png
copying images... [100%] media/homology/klein.png
copying static files... done
dumping search index... done
dumping object inventory... done
build succeeded, 2 warnings.
Build finished. The built documents can be found in /mnt/usb1/scratch/jdemeyer/merger/sage-4.7.3.alpha1/devel/sage/doc/output/html/en/ref
erence
</pre>
TicketSimonKingSat, 22 Oct 2011 21:22:22 GMT
https://trac.sagemath.org/ticket/11115#comment:127
https://trac.sagemath.org/ticket/11115#comment:127
<p>
And how is the docbuild problem related with this patch? It is not clear to me from the docbuild output: Which file has the "unexpected indentation"?
</p>
<p>
But I guess it needs to be rebased against <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a> anyway, isn't it?
</p>
TicketjdemeyerSat, 22 Oct 2011 21:29:23 GMT
https://trac.sagemath.org/ticket/11115#comment:128
https://trac.sagemath.org/ticket/11115#comment:128
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:127" title="Comment 127">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
And how is the docbuild problem related with this patch? It is not clear to me from the docbuild output: Which file has the "unexpected indentation"?
</p>
</blockquote>
<p>
Unfortunately, that's not clear to me either. I was hoping maybe you had an idea, but apparently not.
</p>
<p>
The patches do apply with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>, I don't remember how cleanly.
</p>
TicketSimonKingMon, 24 Oct 2011 08:13:50 GMT
https://trac.sagemath.org/ticket/11115#comment:129
https://trac.sagemath.org/ticket/11115#comment:129
<p>
I can confirm that there <em>is</em> some problem with the docs. I don't know if it is caused by, revealed by, or unrelated with, this patch:
</p>
<pre class="wiki">reading sources... [100%] sage/symbolic/ring
<autodoc>:0: (ERROR/3) Unexpected indentation.
<autodoc>:0: (ERROR/3) Unexpected indentation.
</pre><p>
I wouldn't mind to fix it here. I am only a bit nervous whether I'm shooting myself in the foot: In many of my patches, I am fixing some wrongly formatted doc strings - and it could very well be that this particular problem is already fixed in another of my patches.
</p>
TicketSimonKingMon, 24 Oct 2011 08:21:07 GMT
https://trac.sagemath.org/ticket/11115#comment:130
https://trac.sagemath.org/ticket/11115#comment:130
<p>
Nope. It turned out that the problem is not in sage.symbolic.ring. I touched it, rebuilt, rebuilt the docs: No complaints. Is there a way to find out what file is actually causing trouble?
</p>
TicketSimonKingMon, 24 Oct 2011 09:32:03 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:131
https://trac.sagemath.org/ticket/11115#comment:131
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_info</em>
</li>
</ul>
<p>
Sorry, I can not reliably reproduce it.
</p>
<p>
I applied the patches from here, then did <code>sage -docbuild reference html</code>, and saw the same error that you found. If I am not mistaken, I did not docbuild this installation of Sage before.
</p>
<p>
Since the error seemed to indicate a problem in sage.symbolic.ring, I touched it, rebuilt, and repeated <code>sage -docbuild reference html</code> - but it worked without an error. Hence, it probably isn't sage.symbolic.ring after all.
</p>
<p>
Next, I removed the patches (if you do, don't forget <code>rm build/sage/misc/cachefunc.*</code> and <code>rm build/*/sage/misc/cachefunc.py</code> before <code>sage -b</code>). The docs still built fine.
</p>
<p>
Then, I was applying the first patch from here, and again built sage and the docs - and it worked without problem! The same happened after applying the remaining patches as well.
</p>
<p>
Conclusion: I doubt that my patch is to blame for the docbuild problem.
</p>
<p>
So, how did you come to the conclusion that the problem lies here?
</p>
TicketSimonKingMon, 24 Oct 2011 13:00:04 GMT
https://trac.sagemath.org/ticket/11115#comment:132
https://trac.sagemath.org/ticket/11115#comment:132
<p>
This is what I did to test:
</p>
<ul><li>I downloaded sage-4.7.2.alpha4.tar and built from scratch. Docbuild with <code>sage -docbuild reference html</code> was fine.
</li><li>I applied <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>. After <code>sage -b</code>, I repeated building the reference manual, and it was still fine.
</li><li>I applied <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a> to compensate for the speed regression of <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>, followed by <code>sage -b</code>. The references were still building fine.
</li><li>I applied the four patches from here. For the record: They applied cleanly. <code>sage -br</code> worked, and: Building the references went without problems.
</li></ul><p>
In other words: While I did met a problem when working on top of sage-4.7.2.alpha3, I can not replicate what you found when starting with sage-4.7.2.alpha4.
</p>
<p>
Does that mean the positive review can be restored?
</p>
TicketjdemeyerMon, 24 Oct 2011 13:03:13 GMT
https://trac.sagemath.org/ticket/11115#comment:133
https://trac.sagemath.org/ticket/11115#comment:133
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:132" title="Comment 132">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
In other words: While I did met a problem when working on top of sage-4.7.2.alpha3, I can not replicate what you found when starting with sage-4.7.2.alpha4.
</p>
</blockquote>
<p>
What happens if you first remove all output and then build the documentation:
</p>
<pre class="wiki">$ rm -r SAGE_ROOT/devel/sage/doc/output
$ make doc-html
</pre>
TicketjdemeyerMon, 24 Oct 2011 13:06:07 GMT
https://trac.sagemath.org/ticket/11115#comment:134
https://trac.sagemath.org/ticket/11115#comment:134
<p>
Wild speculation: could it be that your patch indirectly changes docstrings of certain methods?
</p>
<p>
More generally: perhaps the problem is not with a doctest that this patch changed/added but with the docbuilding process? Also because the error is very weird, normally you would indeed get an error with a specific file.
</p>
TicketSimonKingMon, 24 Oct 2011 13:14:59 GMT
https://trac.sagemath.org/ticket/11115#comment:135
https://trac.sagemath.org/ticket/11115#comment:135
<p>
PS: After that, I applied the second patch of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a> and got
</p>
<pre class="wiki">reading sources... [100%] sage/structure/category_object
<autodoc>:0: (ERROR/3) Unexpected indentation.
<autodoc>:0: (ERROR/3) Unexpected indentation.
</pre><p>
So, while <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> seems to be innocent, there does seem to be something wrong in sage.structure.category_object.
</p>
<p>
But the problem remains mysterious. Namely, not only was the resulting reference page looking fine, but also after touching sage/structure/category_object.pyx, <code>sage -b</code> and <code>sage -docbuild reference html</code> worked just fine.
</p>
<p>
I did not try removing all output, though. Doing now.
</p>
TicketSimonKingMon, 24 Oct 2011 13:15:59 GMT
https://trac.sagemath.org/ticket/11115#comment:136
https://trac.sagemath.org/ticket/11115#comment:136
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:134" title="Comment 134">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Wild speculation: could it be that your patch indirectly changes docstrings of certain methods?
</p>
</blockquote>
<p>
Sure. But part of the problem is the fact that we can't locate the error yet: What file is affected? What function/method?
</p>
TicketSimonKingMon, 24 Oct 2011 14:16:52 GMT
https://trac.sagemath.org/ticket/11115#comment:137
https://trac.sagemath.org/ticket/11115#comment:137
<p>
OK, the first step is done: I removed the patches from here, i.e., I have applied
</p>
<pre class="wiki">9138_flat.patch
trac11900_no_categories_for_matrices.patch
trac11900_category_speedup.patch
trac11900_further_tweaks.patch
</pre><p>
on top of sage-4.7.2.alpha4. After removing the doc output and doing <code>make doc-html</code>, all docs built fine.
</p>
<p>
Now, I am applying the patches from here, <code>sage -b</code>, then I'll remove the output again, start building the docs, and go drink a coffee (building the docs takes an awful lot of time!).
</p>
TicketSimonKingMon, 24 Oct 2011 15:15:58 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:138
https://trac.sagemath.org/ticket/11115#comment:138
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:137" title="Comment 137">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Now, I am applying the patches from here, <code>sage -b</code>, then I'll remove the output again, start building the docs, and go drink a coffee (building the docs takes an awful lot of time!).
</p>
</blockquote>
<p>
After the coffee, I found:
</p>
<pre class="wiki">king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4/devel/sage$ make doc-html
...
build succeeded.
Build finished. The built documents can be found in /mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4/devel/sage/doc/output/html/ru/tutorial
king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4$ cd devel/sage
king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4/devel/sage$ hg qapplied
9138_flat.patch
trac11900_no_categories_for_matrices.patch
trac11900_category_speedup.patch
trac11900_further_tweaks.patch
trac11115-cached_cython.patch
trac11115_element_with_cache.patch
trac11115_cached_function_pickling.patch
trac_11115_reviewer.patch
</pre><p>
Hence, I definitely can <em>not</em> reproduce your findings, at least not with sage-4.7.2.alpha4.
</p>
TicketjdemeyerMon, 24 Oct 2011 15:22:04 GMT
https://trac.sagemath.org/ticket/11115#comment:139
https://trac.sagemath.org/ticket/11115#comment:139
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:138" title="Comment 138">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:137" title="Comment 137">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Now, I am applying the patches from here, <code>sage -b</code>, then I'll remove the output again, start building the docs, and go drink a coffee (building the docs takes an awful lot of time!).
</p>
</blockquote>
<p>
After the coffee, I found:
</p>
<pre class="wiki">king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4/devel/sage$ make doc-html
...
build succeeded.
Build finished. The built documents can be found in /mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4/devel/sage/doc/output/html/ru/tutorial
king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4$ cd devel/sage
king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4/devel/sage$ hg qapplied
9138_flat.patch
trac11900_no_categories_for_matrices.patch
trac11900_category_speedup.patch
trac11900_further_tweaks.patch
trac11115-cached_cython.patch
trac11115_element_with_cache.patch
trac11115_cached_function_pickling.patch
trac_11115_reviewer.patch
</pre><p>
Hence, I definitely can <em>not</em> reproduce your findings, at least not with sage-4.7.2.alpha4.
</p>
</blockquote>
<p>
Did you double-check that there is no error in the log file?
</p>
<pre class="wiki">$ grep -i error dochtml.log
</pre><p>
(note that the log file is always <em>appended</em> to, so you need to find the last <code>make doc-html</code> run within the logfile).
</p>
TicketSimonKingMon, 24 Oct 2011 15:35:49 GMT
https://trac.sagemath.org/ticket/11115#comment:140
https://trac.sagemath.org/ticket/11115#comment:140
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:139" title="Comment 139">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
(note that the log file is always <em>appended</em> to, so you need to find the last <code>make doc-html</code> run within the logfile).
</p>
</blockquote>
<p>
So, it would be very difficult to find out which run worked and which didn't. Unfortunately, before reading your post, I have already started building docs with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>. Thus, the log for <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> is not the last part but the last but one part of the log file. And I already saw on screen that with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a> I am again getting the ominous doc build error.
</p>
<p>
I must admit, it confuses me.
</p>
TicketSimonKingMon, 24 Oct 2011 15:49:32 GMT
https://trac.sagemath.org/ticket/11115#comment:141
https://trac.sagemath.org/ticket/11115#comment:141
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:139" title="Comment 139">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Did you double-check that there is no error in the log file?
</p>
<pre class="wiki">$ grep -i error dochtml.log
</pre></blockquote>
<p>
Will dochtml.log contain exactly the same that is printed on screen? Because I did scroll the screen up and did not see any error. But I think, for being on the safe side, I will return to <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, remove the output, remove dochtml.log, and build the docs again - because in that case I can be sure that the log file only contains data of one run.
</p>
TicketSimonKingMon, 24 Oct 2011 16:46:55 GMT
https://trac.sagemath.org/ticket/11115#comment:142
https://trac.sagemath.org/ticket/11115#comment:142
<p>
First part: With <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a> only and a clean dochtml.log, I have
</p>
<pre class="wiki">king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4$ grep -i error dochtml.log
reading sources... [ 79%] sage/rings/padics/precision_error
writing output... [ 79%] sage/rings/padics/precision_error
</pre><p>
So, no error prior to <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>.
</p>
TicketSimonKingTue, 25 Oct 2011 05:42:49 GMT
https://trac.sagemath.org/ticket/11115#comment:143
https://trac.sagemath.org/ticket/11115#comment:143
<p>
Now I can finally confirm the error: After applying <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> and rebuilding the docs, I find
</p>
<pre class="wiki">king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4$ grep -i error dochtml.log
reading sources... [ 79%] sage/rings/padics/precision_error
<autodoc>:0: (ERROR/3) Unexpected indentation.
<autodoc>:0: (ERROR/3) Unexpected indentation.
writing output... [ 79%] sage/rings/padics/precision_error
</pre><p>
I think it is very likely that the problem occurs in the documentation of a cached method. Hence, I will modify the _sage_doc_ method of cached methods, so that the doc string will be stored in some log file - and then, I can see what's wrong.
</p>
TicketSimonKingTue, 25 Oct 2011 06:37:26 GMT
https://trac.sagemath.org/ticket/11115#comment:144
https://trac.sagemath.org/ticket/11115#comment:144
<p>
Debugging will not be totally easy. Apparently, 1153 cached methods/functions appear in the docs. I hope I can find the two needles in that haystack...
</p>
TicketSimonKingTue, 25 Oct 2011 10:15:48 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:145
https://trac.sagemath.org/ticket/11115#comment:145
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
It is frustrating. I made _sage_doc_ of cached functions and methods print all doc strings into a file, and looking over the 1153 instances (some of them with repetition) actually made me fix some misformatted doc strings.
</p>
<p>
However, the two needles have not been among them.
</p>
TicketSimonKingTue, 25 Oct 2011 10:29:01 GMT
https://trac.sagemath.org/ticket/11115#comment:146
https://trac.sagemath.org/ticket/11115#comment:146
<p>
Good and bad news: When I collected all 1153 doc strings into a single test file and built its documentation, there was no "unexpected unindent". Hence, cached methods or functions are not to blame.
</p>
<p>
The bad news is that I can not locate the error. I have to carefully read my patches again. Perhaps there is a change to the _sage_doc_ of a different method decorator?
</p>
TicketSimonKingTue, 25 Oct 2011 10:46:00 GMT
https://trac.sagemath.org/ticket/11115#comment:147
https://trac.sagemath.org/ticket/11115#comment:147
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:146" title="Comment 146">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
I have to carefully read my patches again. Perhaps there is a change to the _sage_doc_ of a different method decorator?
</p>
</blockquote>
<p>
Bad luck. Searching for "doc" in all patches showed me that the doctest behaviour is changed only in one place: _sage_doc_ for cached functions/methods. And my previous attempts to track down any wrongly formatted doc strings returned by _sage_doc_ have failed.
</p>
TicketSimonKingTue, 25 Oct 2011 14:21:23 GMT
https://trac.sagemath.org/ticket/11115#comment:148
https://trac.sagemath.org/ticket/11115#comment:148
<p>
I am now trying a more radical method to find out what happened: I've built the documentation with the first patch (the first patch is responsible for the two errors), moved it to a different location, then built the documentation without the patch, and now I am doing a kdiff3.
</p>
<p>
There are many differenced, but I hope kdiff3 will relatively easily tell me what really went wrong.
</p>
TicketSimonKingTue, 25 Oct 2011 14:26:04 GMT
https://trac.sagemath.org/ticket/11115#comment:149
https://trac.sagemath.org/ticket/11115#comment:149
<p>
And I already got something that looks wrong!
</p>
<p>
The doc string of modp_splitting_data starts with "Return mod p splitting data for this quaternion algebra at " - and this half sentence is missing with my patch! Mysterious.
</p>
TicketSimonKingTue, 25 Oct 2011 15:48:32 GMT
https://trac.sagemath.org/ticket/11115#comment:150
https://trac.sagemath.org/ticket/11115#comment:150
<p>
The doc for the reference manual is obtained by <code>_sage_getdoc_unformatted</code>. The difference in the output with or without my patch is that the docstring contains embedding information with my patch. That is important for introspection - otherwise, one has
</p>
<pre class="wiki">sage: sage_getfile(Q.modp_splitting_data)
'/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha3/local/lib/python2.6/site-packages/sage/misc/cachefunc.py'
</pre><p>
Having the embedding information (such as <code>'File: sage/algebras/quatalg/quaternion_algebra.py (starting at line 859)'</code>) is a very usual thing. If I am not mistaken, it occurs for any extension class. So, I am very surprised to see that this is a problem.
</p>
<p>
Actually, it seems that the first line of the doc string is systematically suppressed for cached methods - the same happens for <code>groebner_basis()</code> and other cached methods.
</p>
<p>
So, that looks to me like a bug in pre-processing the doc strings for building the references.
</p>
TicketSimonKingTue, 25 Oct 2011 15:52:17 GMT
https://trac.sagemath.org/ticket/11115#comment:151
https://trac.sagemath.org/ticket/11115#comment:151
<p>
Wow! It seems that the embedding information for an extension class is always followed by a blank line - which is not the case for cached methods, with my patch.
</p>
<p>
I bet that the problem will be solved by simply inserting a blank line after the embedding information!
</p>
TicketSimonKingTue, 25 Oct 2011 16:45:34 GMTstatus, description changed
https://trac.sagemath.org/ticket/11115#comment:152
https://trac.sagemath.org/ticket/11115#comment:152
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=152">diff</a>)
</li>
</ul>
<p>
Hooray! With the blank line, the missing first lines of the <code>modp_splitting_data</code> doc string (and other instances) re-appear, and most importantly the error has vanished:
</p>
<pre class="wiki">king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.7.2.alpha4$ grep -i error dochtml.log
reading sources... [ 79%] sage/rings/padics/precision_error
writing output... [ 79%] sage/rings/padics/precision_error
</pre><p>
The new patch does the following:
</p>
<ul><li>Fix some wrong docstring formats that I stumbled over.
</li><li>Add the required blank line after the embedding information in the doc strings of cached methods.
</li><li>In two cases, a cached function has been created by first defining a non-cached function, e.g. <code>adem_</code>, and then defining <code>adem = CachedFunction(adem_)</code>. If I am not mistaken, <em>both</em> functions would be included in the docs, which is not good. So, for aesthetic reasons, I rewrote it using <code>@cached_function</code>.
</li></ul><p>
I am now running doc tests. It could be that some tests in sage_inspect will fail. But I am confident enough to turn it into "needs review".
</p>
<p>
For the patch bot:
</p>
<p>
Apply trac11115-cached_cython.patch trac11115_element_with_cache.patch trac11115_cached_function_pickling.patch trac_11115_reviewer.patch trac_11115_docfix.patch
</p>
TicketSimonKingTue, 25 Oct 2011 17:58:38 GMT
https://trac.sagemath.org/ticket/11115#comment:153
https://trac.sagemath.org/ticket/11115#comment:153
<p>
FWIW: All tests pass!
</p>
<p>
So, I hope that we will soon find someone who restores the positive review. But please have a look at the last patch!
</p>
TicketjdemeyerWed, 02 Nov 2011 10:32:33 GMTdependencies, description changed; work_issues deleted
https://trac.sagemath.org/ticket/11115#comment:154
https://trac.sagemath.org/ticket/11115#comment:154
<ul>
<li><strong>dependencies</strong>
changed from <em>#9976 #11298 #11342 #9138 #11815</em> to <em>#9976, #11298, #11342, #9138, #11815, #11972</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=154">diff</a>)
</li>
<li><strong>work_issues</strong>
<em>docbuild</em> deleted
</li>
</ul>
<p>
First four patches flattened and rebased.
</p>
TicketjdemeyerWed, 02 Nov 2011 10:33:33 GMTattachment set
https://trac.sagemath.org/ticket/11115
https://trac.sagemath.org/ticket/11115
<ul>
<li><strong>attachment</strong>
set to <em>11115_flat.patch</em>
</li>
</ul>
TicketjdemeyerWed, 02 Nov 2011 13:52:43 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:155
https://trac.sagemath.org/ticket/11115#comment:155
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:153" title="Comment 153">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
FWIW: All tests pass!
</p>
</blockquote>
<p>
I get errors with long tests, because of the mismatch between <code>steenrod_algebra_basis_</code> and <code>steenrod_algebra_basis</code>:
</p>
<pre class="wiki">sage -t -long -force_lib devel/sage/sage/algebras/steenrod/steenrod_algebra_bases.py
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.3.alpha1/devel/sage-main/sage/algebras/steenrod/steenrod_algebra_bases.py", line 1125:
sage: steenrod_basis_error_check(15,2) # long time
Exception raised:
Traceback (most recent call last):
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.3.alpha1/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.3.alpha1/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.3.alpha1/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_12[3]>", line 1, in <module>
steenrod_basis_error_check(Integer(15),Integer(2)) # long time###line 1125:
sage: steenrod_basis_error_check(15,2) # long time
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.3.alpha1/local/lib/python/site-packages/sage/algebras/steenrod/steenrod_algebra_bases.py", line 1143, in steenrod_basis_error_check
milnor_dim = len(steenrod_algebra_basis_(i,'milnor',p=p))
NameError: global name 'steenrod_algebra_basis_' is not defined
**********************************************************************
</pre><p>
(2 similar errors in the same file)
</p>
TicketSimonKingWed, 02 Nov 2011 14:28:59 GMT
https://trac.sagemath.org/ticket/11115#comment:156
https://trac.sagemath.org/ticket/11115#comment:156
<p>
Hi Jeroen,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:155" title="Comment 155">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:153" title="Comment 153">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
FWIW: All tests pass!
</p>
</blockquote>
<p>
I get errors with long tests, because of the mismatch between <code>steenrod_algebra_basis_</code> and <code>steenrod_algebra_basis</code>:
</p>
</blockquote>
<p>
Is that because of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11972" title="enhancement: avoid race conditions when creating directories (closed: fixed)">#11972</a>, that you added as a dependency?
</p>
TicketjdemeyerWed, 02 Nov 2011 14:38:20 GMT
https://trac.sagemath.org/ticket/11115#comment:157
https://trac.sagemath.org/ticket/11115#comment:157
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:156" title="Comment 156">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Is that because of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11972" title="enhancement: avoid race conditions when creating directories (closed: fixed)">#11972</a>, that you added as a dependency?
</p>
</blockquote>
<p>
I don't think so, it is because of <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11115/trac_11115_docfix.patch" title="Attachment 'trac_11115_docfix.patch' in Ticket #11115">trac_11115_docfix.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11115/trac_11115_docfix.patch" title="Download"></a>
</p>
TicketSimonKingWed, 02 Nov 2011 15:36:16 GMT
https://trac.sagemath.org/ticket/11115#comment:158
https://trac.sagemath.org/ticket/11115#comment:158
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:155" title="Comment 155">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:153" title="Comment 153">SimonKing</a>:
</p>
<blockquote>
<p>
sage: steenrod_basis_error_check(15,2) # long time
</p>
</blockquote>
</blockquote>
<p>
Ah, I see! I wasn't running the long tests. And apparently that long tests is internally calling the uncached function (which probably is a bad idea and ought to change anyway).
</p>
TicketSimonKingWed, 02 Nov 2011 16:14:45 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:159
https://trac.sagemath.org/ticket/11115#comment:159
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I have updated the docfix patch. The new patch also removes references to the old uncached underscore versions of the cached functions in the references.
</p>
<p>
I find
</p>
<pre class="wiki">../../sage -t -long sage/algebras/steenrod/
sage -t -long "devel/sage-main/sage/algebras/steenrod/steenrod_algebra_bases.py"
[15.0 s]
sage -t -long "devel/sage-main/sage/algebras/steenrod/steenrod_algebra_misc.py"
[2.2 s]
sage -t -long "devel/sage-main/sage/algebras/steenrod/all.py"
[2.2 s]
sage -t -long "devel/sage-main/sage/algebras/steenrod/steenrod_algebra_mult.py"
[2.2 s]
sage -t -long "devel/sage-main/sage/algebras/steenrod/steenrod_algebra.py"
[36.7 s]
sage -t -long "devel/sage-main/sage/algebras/steenrod/__init__.py"
[0.1 s]
----------------------------------------------------------------------
All tests passed!
</pre><p>
Apply 11115_flat.patch trac_11115_docfix.patch
</p>
TicketjdemeyerThu, 03 Nov 2011 08:45:35 GMT
https://trac.sagemath.org/ticket/11115#comment:160
https://trac.sagemath.org/ticket/11115#comment:160
<p>
All tests pass indeed.
</p>
TicketjdemeyerWed, 09 Nov 2011 13:25:24 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:161
https://trac.sagemath.org/ticket/11115#comment:161
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
There are some problems with the new Cython (<a class="closed ticket" href="https://trac.sagemath.org/ticket/11761" title="enhancement: Upgrade Cython to 0.15.1 (closed: fixed)">#11761</a>, to be merged in sage-4.8.alpha1), the following fails:
</p>
<pre class="wiki">sage -t -force_lib devel/sage/sage/misc/cachefunc.pyx
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha2/devel/sage-main/sage/misc/cachefunc.pyx", line 97:
sage: cython('\n'.join(cython_code))
Exception raised:
Traceback (most recent call last):
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha2/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha2/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha2/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_0[18]>", line 1, in <module>
cython('\n'.join(cython_code))###line 97:
sage: cython('\n'.join(cython_code))
File "cython_c.pyx", line 61, in sage.misc.cython_c.cython (sage/misc/cython_c.c:688)
sage.server.support.cython_import_all(tmpfile, globals(),
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha2/local/lib/python/site-packages/sage/server/support.py", line 473, in cython_import_all
create_local_c_file=create_local_c_file)
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha2/local/lib/python/site-packages/sage/server/support.py", line 450, in cython_import
**kwds)
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha2/local/lib/python/site-packages/sage/misc/cython.py", line 403, in cython
raise RuntimeError, "Error converting %s to C:\n%s\n%s"%(filename, log, err)
RuntimeError: Error converting /scratch/jdemeyer/merger/sage-4.8.alpha2/home/.sage//temp/sage.math.washington.edu/5609//tmp_2.spyx to C:
Error compiling Cython file:
------------------------------------------------------------
...
include "cdefs.pxi"
from sage.all import cached_method, cached_in_parent_method, Category
class MyCategory(Category):
@cached_method
def super_categories(self):
return [Objects()]
^
------------------------------------------------------------
_scratch_jdemeyer_merger_sage_4_8_alpha2_home__sage_temp_sage_math_washington_edu_5609_tmp_2_spyx_0.pyx:10:23: undeclared name not builtin: Objects
**********************************************************************
</pre>
TicketSimonKingWed, 09 Nov 2011 14:19:41 GMT
https://trac.sagemath.org/ticket/11115#comment:162
https://trac.sagemath.org/ticket/11115#comment:162
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:161" title="Comment 161">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
There are some problems with the new Cython (<a class="closed ticket" href="https://trac.sagemath.org/ticket/11761" title="enhancement: Upgrade Cython to 0.15.1 (closed: fixed)">#11761</a>, to be merged in sage-4.8.alpha1), the following fails:
</p>
<blockquote>
<p>
include "cdefs.pxi"
from sage.all import cached_method, cached_in_parent_method, Category
class <a class="missing wiki">MyCategory?</a>(Category):
</p>
<blockquote>
<p>
@cached_method
def super_categories(self):
</p>
<blockquote>
<p>
return [Objects()]
</p>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<p>
It is actually surprising that this used to work. After all, Objects has not been imported.
</p>
TicketSimonKingWed, 09 Nov 2011 14:36:14 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:163
https://trac.sagemath.org/ticket/11115#comment:163
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I have added the import statement to the second patch, and obtain:
</p>
<pre class="wiki">king@mpc622:/mnt/local/king/SAGE/rebase/sage-4.8.alpha0/devel/sage$ ../../sage -t sage/misc/cachefunc.pyx
sage -t "devel/sage-main/sage/misc/cachefunc.pyx"
[13.0 s]
----------------------------------------------------------------------
All tests passed!
</pre>
TicketSimonKingWed, 09 Nov 2011 16:54:33 GMT
https://trac.sagemath.org/ticket/11115#comment:164
https://trac.sagemath.org/ticket/11115#comment:164
<p>
I wonder: Isn't the same doc test also in other files? sage/structure/element.pyx? I have to leave now, but will check it.
</p>
TicketSimonKingWed, 09 Nov 2011 19:08:52 GMTattachment set
https://trac.sagemath.org/ticket/11115
https://trac.sagemath.org/ticket/11115
<ul>
<li><strong>attachment</strong>
set to <em>trac_11115_docfix.patch</em>
</li>
</ul>
<p>
Fix some doc strings, and add the required blank line after embedding information for cached methods
</p>
TicketSimonKingWed, 09 Nov 2011 19:13:05 GMT
https://trac.sagemath.org/ticket/11115#comment:165
https://trac.sagemath.org/ticket/11115#comment:165
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:164" title="Comment 164">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
I wonder: Isn't the same doc test also in other files? sage/structure/element.pyx? I have to leave now, but will check it.
</p>
</blockquote>
<p>
Yes, the same problem occurs in sage/structure/element.pyx. It is fixed with the new version of the docfix patch that I have just attached. At least parent.pyx, element.pyx and cachefunc.pyx pass, and I am now running all doc tests.
</p>
<p>
But I think it is ok to keep the status "needs review". Recall: The review concerns the docfix patch, the first patch already has a positive review.
</p>
TicketSimonKingWed, 09 Nov 2011 20:11:22 GMT
https://trac.sagemath.org/ticket/11115#comment:166
https://trac.sagemath.org/ticket/11115#comment:166
<p>
FWIW: <code>./sage -tp 6 -long devel/sage/sage/</code> passes for me!
</p>
TicketSimonKingWed, 23 Nov 2011 15:56:37 GMT
https://trac.sagemath.org/ticket/11115#comment:167
https://trac.sagemath.org/ticket/11115#comment:167
<p>
Is there no reviewer for the docfix patch, so that we can finally have fast cached methods?
</p>
TicketnovoseltWed, 23 Nov 2011 17:24:01 GMT
https://trac.sagemath.org/ticket/11115#comment:168
https://trac.sagemath.org/ticket/11115#comment:168
<p>
The doctest patch looks fine to me. But as I understand it, before this one is switched to positive review, its implicit dependency <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a> must be approved.
</p>
TicketSimonKingWed, 23 Nov 2011 18:12:32 GMT
https://trac.sagemath.org/ticket/11115#comment:169
https://trac.sagemath.org/ticket/11115#comment:169
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:168" title="Comment 168">novoselt</a>:
</p>
<blockquote class="citation">
<p>
The doctest patch looks fine to me. But as I understand it, before this one is switched to positive review, its implicit dependency <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a> must be approved.
</p>
</blockquote>
<p>
Of course, it can only be <em>merged</em> if all dependencies have been merged. But as far as I understand (I know several examples), a patch can get a positive <em>review</em> even when one of its dependencies needs work.
</p>
<p>
But perhaps the release manager can tell more?
</p>
TicketvbraunWed, 23 Nov 2011 18:43:32 GMTstatus changed
https://trac.sagemath.org/ticket/11115#comment:170
https://trac.sagemath.org/ticket/11115#comment:170
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
After you review it positively you set it to positive review :-)
</p>
TicketSimonKingWed, 23 Nov 2011 20:23:06 GMT
https://trac.sagemath.org/ticket/11115#comment:171
https://trac.sagemath.org/ticket/11115#comment:171
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:170" title="Comment 170">vbraun</a>:
</p>
<blockquote class="citation">
<p>
After you review it positively you set it to positive review :-)
</p>
</blockquote>
<p>
Just to make sure: Andrey, did you really intend to set it to positive review? Or did you merely say "the second patch seems OK, but for a review I need to look more closely"?
</p>
TicketnovoseltWed, 23 Nov 2011 20:40:58 GMT
https://trac.sagemath.org/ticket/11115#comment:172
https://trac.sagemath.org/ticket/11115#comment:172
<p>
Well, I didn't run tests yet, but assuming that patches apply and doctests pass, it is positive review. I read the second patch and don't think that anything has to be changed in it.
</p>
TicketnovoseltThu, 24 Nov 2011 00:58:48 GMT
https://trac.sagemath.org/ticket/11115#comment:173
https://trac.sagemath.org/ticket/11115#comment:173
<p>
And I confirm that all long tests do pass on sage-4.8.alpha2. I had fuzz with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>, but that's the one which is still in the works. Yay to standard cached methods, they are much more pleasant to work with than catching <code>AttributeError</code>!
</p>
TicketjdemeyerSat, 26 Nov 2011 13:13:25 GMTdependencies changed
https://trac.sagemath.org/ticket/11115#comment:174
https://trac.sagemath.org/ticket/11115#comment:174
<ul>
<li><strong>dependencies</strong>
changed from <em>#9976, #11298, #11342, #9138, #11815, #11972</em> to <em>#9138, #11900</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:169" title="Comment 169">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Of course, it can only be <em>merged</em> if all dependencies have been merged. But as far as I understand (I know several examples), a patch can get a positive <em>review</em> even when one of its dependencies needs work.
</p>
<p>
But perhaps the release manager can tell more?
</p>
</blockquote>
<p>
What Simon said is exactly right: a patch can get positive_review even when the dependencies do not yet have positive_review.
</p>
<p>
I really hope that <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a> can get reviewed soon, it would be nice indeed to have <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> in Sage.
</p>
TicketjdemeyerSat, 26 Nov 2011 13:14:11 GMTdescription changed
https://trac.sagemath.org/ticket/11115#comment:175
https://trac.sagemath.org/ticket/11115#comment:175
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11115?action=diff&version=175">diff</a>)
</li>
</ul>
TicketnthierySat, 26 Nov 2011 17:45:43 GMT
https://trac.sagemath.org/ticket/11115#comment:176
https://trac.sagemath.org/ticket/11115#comment:176
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:174" title="Comment 174">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
I really hope that <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a> can get reviewed soon, it would be nice indeed to have <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> in Sage.
</p>
</blockquote>
<p>
I have been through most of that patch with Simon, and I just now need to make a final pass. I had planned to do it on Friday, but got delayed by some sagetex hacking for the upcoming new version of the French sagebook. Most likely, I'll get to do it next Tuesday.
</p>
<p>
Cheers,
</p>
<blockquote>
<p>
Nicolas
</p>
</blockquote>
TicketjdemeyerFri, 09 Dec 2011 23:57:22 GMTmilestone changed
https://trac.sagemath.org/ticket/11115#comment:177
https://trac.sagemath.org/ticket/11115#comment:177
<ul>
<li><strong>milestone</strong>
changed from <em>sage-pending</em> to <em>sage-5.0</em>
</li>
</ul>
TicketjdemeyerWed, 18 Jan 2012 08:08:37 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/11115#comment:178
https://trac.sagemath.org/ticket/11115#comment:178
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.0.beta0</em>
</li>
</ul>
TicketjdemeyerThu, 19 Feb 2015 15:53:15 GMT
https://trac.sagemath.org/ticket/11115#comment:179
https://trac.sagemath.org/ticket/11115#comment:179
<p>
Could you please explain this line in <code>src/sage/misc/cachefunc.pxd</code>?
</p>
<pre class="wiki"># cache is not always of type "dict"!
</pre><p>
It seems that the implementation assumes that <code>cache</code> is a <code>dict</code> anyway (I see lots of <code><dict>self.cache</code> in the code).
</p>
TicketSimonKingThu, 19 Feb 2015 17:41:02 GMT
https://trac.sagemath.org/ticket/11115#comment:180
https://trac.sagemath.org/ticket/11115#comment:180
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:179" title="Comment 179">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Could you please explain this line in <code>src/sage/misc/cachefunc.pxd</code>?
</p>
<pre class="wiki"># cache is not always of type "dict"!
</pre></blockquote>
<p>
That's why:
</p>
<pre class="wiki">sage: from weakref import WeakValueDictionary
sage: W = WeakValueDictionary()
sage: isinstance(W, dict)
False
</pre><p>
We use weak value dictionaries for weak_cached_function.
</p>
<blockquote class="citation">
<p>
It seems that the implementation assumes that <code>cache</code> is a <code>dict</code> anyway (I see lots of <code><dict>self.cache</code> in the code).
</p>
</blockquote>
<p>
But certainly not in weak_cached_function.
</p>
TicketjdemeyerThu, 19 Feb 2015 21:04:36 GMT
https://trac.sagemath.org/ticket/11115#comment:181
https://trac.sagemath.org/ticket/11115#comment:181
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:180" title="Comment 180">SimonKing</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
It seems that the implementation assumes that <code>cache</code> is a <code>dict</code> anyway (I see lots of <code><dict>self.cache</code> in the code).
</p>
</blockquote>
<p>
But certainly not in weak_cached_function.
</p>
</blockquote>
<p>
I think that's a bad design with all the <code><dict></code> casts. I would have prefered something more like:
</p>
<pre class="wiki">class BaseCachedFunction(object):
pass
class CachedFunction(BaseCachedFunction):
cdef dict cache
class WeakCachedFunction(BaseCachedFunction):
cdef WeakValueDictionary cache
</pre><p>
This is like the classes for dense/sparse vectors.
</p>
TicketSimonKingThu, 19 Feb 2015 21:27:21 GMT
https://trac.sagemath.org/ticket/11115#comment:182
https://trac.sagemath.org/ticket/11115#comment:182
<p>
As it seems, there will be a new ticket on cached methods soon, because of the sad fact that removing a cython source file from the Sage library does not only result in problems with introspection of cached methods, but the cached methods can't even be called. So, I suppose the design flaw can be addressed there, too.
</p>
TicketjdemeyerFri, 20 Feb 2015 10:56:47 GMT
https://trac.sagemath.org/ticket/11115#comment:183
https://trac.sagemath.org/ticket/11115#comment:183
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:182" title="Comment 182">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
As it seems, there will be a new ticket on cached methods soon, because of the sad fact that removing a cython source file from the Sage library does not only result in problems with introspection of cached methods, but the cached methods can't even be called. So, I suppose the design flaw can be addressed there, too.
</p>
</blockquote>
<p>
Unless <a class="closed ticket" href="https://trac.sagemath.org/ticket/17814" title="defect: Make calling a cached method independent of source code inspection (closed: fixed)">#17814</a> really requires major refactoring anyway, I wouldn't do it on the same ticket.
</p>
<p>
Also, I certainly don't want to force you to "fix" this. It works as it is.
</p>
TicketSimonKingFri, 20 Feb 2015 11:08:21 GMT
https://trac.sagemath.org/ticket/11115#comment:184
https://trac.sagemath.org/ticket/11115#comment:184
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11115#comment:183" title="Comment 183">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Unless <a class="closed ticket" href="https://trac.sagemath.org/ticket/17814" title="defect: Make calling a cached method independent of source code inspection (closed: fixed)">#17814</a> really requires major refactoring anyway, I wouldn't do it on the same ticket.
</p>
</blockquote>
<p>
Actually, it currently seems that this would better be addressed by changing sage.misc.sageinspect.
</p>
TicketSimonKingFri, 20 Feb 2015 11:28:01 GMT
https://trac.sagemath.org/ticket/11115#comment:185
https://trac.sagemath.org/ticket/11115#comment:185
<p>
Hm. Fixing sageinspect won't be easy. I currently have absolutely no clue how to read off the number of arguments (or even the names of the arguments) of a function that is defined in a Cython file. Those functions don't seem to have any attributes holding useful information.
</p>
<p>
Has someone else an idea? If not, then I don't see what we can do. We do want a special cached method implementation for methods without arguments, and thus we need to determine the number of arguments of the to-be-wrapped method. If that doesn't work without reading the sources, what could we possibly do?
</p>
TicketjdemeyerWed, 30 Dec 2015 15:04:42 GMT
https://trac.sagemath.org/ticket/11115#comment:186
https://trac.sagemath.org/ticket/11115#comment:186
<p>
What's the purpose of this opaque block of code?
</p>
<pre class="wiki"> if args:
lenargs = len(args)
nargs = self._argument_fixer._nargs
if self._inst_in_key:
if lenargs>=nargs:
k = (self._instance,(args,()))
else:
k = (self._instance,(<tuple>args+(<tuple>self._argument_fixer._default_tuple)[-nargs+lenargs:],()))
else:
if lenargs>=nargs:
k = (args,())
else:
k = (<tuple>args+(<tuple>self._argument_fixer._default_tuple)[-nargs+lenargs:],())
</pre>
Ticket