Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#15692 closed defect (fixed)

Value of cached methods should not always be pickled

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-7.0
Component: pickling Keywords: pickling, hash, cache, days71
Cc: ncohen, vbraun, nbruin, novoselt, jkeitel, nthiery Merged in:
Authors: Julian Rüth Reviewers: Vincent Delecroix, David Roe
Report Upstream: N/A Work issues:
Branch: a18cbf5 (Commits, GitHub, GitLab) Commit:
Dependencies: #16337 Stopgaps:

Status badges

Description

In #15278, the hash of graphs was turned into a @cached_method. If a cached method is pickled, then the cached value is pickled as well. In the case of a hash value, this is not always what we want, since the hash value might depend on the memory address of an object, or worse: The hash function might change in future implementations.

That's to say: If you save object O now, together with hash value 123, and later load it, then hash(O) will still be 123, even if a newly constructed object P with O==P might have hash(P)==456, because of a change in the hash function.

This obviously is a problem. There is something called sage.misc.cachefunc.ClearCacheOnPickle, but I don't know if this would really help in this case. Also, this would clear all cache values at once. Also, I don't know if it works for cached methods that do not take arguments (beside self).

I suggest that we should instead have an optional parameter for sage.misc.cachefunc.CachedMethodCallerNoArgs that determines whether or not the cached value is preserved, and use it on graphs (and future applications of cached method on __hash__).

Change History (69)

comment:1 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:2 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:3 Changed 7 years ago by saraedum

  • Cc vbraun nbruin novoselt jkeitel added

In #15149 it was proposed that caches should be 'opt-in and not opt-out' (comment:6:ticket:15149). Imho most caches are not used for things that are horribly expensive to compute, they are just too expensive to be computed again all the time.

Would somebody mind if I implemented it that way?

Version 0, edited 7 years ago by saraedum (next)

comment:4 Changed 7 years ago by saraedum

  • Branch set to u/saraedum/ticket/15692
  • Created changed from 01/18/14 09:30:18 to 01/18/14 09:30:18
  • Modified changed from 05/12/14 10:05:02 to 05/12/14 10:05:02

comment:5 Changed 7 years ago by saraedum

  • Dependencies set to #16337

comment:6 Changed 7 years ago by saraedum

  • Authors set to Julian Rueth
  • Modified changed from 05/12/14 17:43:50 to 05/12/14 17:43:50
  • Status changed from new to needs_review

comment:7 Changed 7 years ago by novoselt

  • Commit set to 07178496787dfa2e02b7bab8e32ca8e2971edaba

I'm in support of this although I am not the right person to review the changes.


New commits:

ce27b84Removed sage.misc.cachefunc.ClearCacheOnPickle
e27e316Propagate key of a @cached_method correctly
94fcddbMerge branch 'u/saraedum/ticket/16337' of git://trac.sagemath.org/sage into ticket/15692
72fce8bAdded a pickle parameter for @cached_method
0717849Enable pickling of the cache for groebner_basis()

comment:8 Changed 7 years ago by git

  • Commit changed from 07178496787dfa2e02b7bab8e32ca8e2971edaba to 9eab84b8c4f377576d067a9b7ca0eba43244f317

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

9eab84bMerge branch 'develop' into ticket/15692

comment:9 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:10 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply, needs rebase

comment:11 Changed 6 years ago by git

  • Commit changed from 9eab84b8c4f377576d067a9b7ca0eba43244f317 to 13b4e4cd7dc1ebd4f524dbe9cb33fb2a8775c3d6

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

13b4e4cMerge branch 'develop' into t/15692/ticket/15692

comment:12 Changed 6 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:13 Changed 6 years ago by chapoton

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

one failing doctest, see patchbot report

comment:14 Changed 6 years ago by git

  • Commit changed from 13b4e4cd7dc1ebd4f524dbe9cb33fb2a8775c3d6 to 616e86189f0d3a30c4885e38e01a619005446855

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

616e861Fix doctest in sage/misc/cachefunc.pyx

comment:15 Changed 6 years ago by saraedum

  • Status changed from needs_work to needs_review
Last edited 6 years ago by saraedum (previous) (diff)

comment:16 follow-up: Changed 6 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Hello,

In _cached_function_unpickle instead of

for k,v in cache:
    ret.cache[k] = v

you can simply do ret.cache.update(cache). Moreover, in that function you should document the argument cache.

I think that disabling the cache by default on pickle is a good improvement. However, the class ClearCacheOnPickle that you remove in your branch had several purposes: clear the cache on pickle but also on copy. Your version keep the cache on copy and that might not be appropriate for classes that used ClearCacheOnPickle.

Vincent

comment:17 Changed 6 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix

comment:18 Changed 6 years ago by git

  • Commit changed from 616e86189f0d3a30c4885e38e01a619005446855 to b2aa4073a6724e0c92c719f8d3eb232ef0bc2102

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

b2aa407Improve docstring of sage.misc.cachefunc._cached_function_unpickle

comment:19 in reply to: ↑ 16 ; follow-up: Changed 6 years ago by saraedum

Replying to vdelecroix:

I think that disabling the cache by default on pickle is a good improvement. However, the class ClearCacheOnPickle that you remove in your branch had several purposes: clear the cache on pickle but also on copy. Your version keep the cache on copy and that might not be appropriate for classes that used ClearCacheOnPickle.

Thanks for pointing this out. What should we do about this? Should caches ever be copied actually?

comment:20 Changed 6 years ago by saraedum

  • Status changed from needs_work to needs_info

comment:21 in reply to: ↑ 19 ; follow-up: Changed 6 years ago by vdelecroix

Replying to saraedum:

Replying to vdelecroix:

I think that disabling the cache by default on pickle is a good improvement. However, the class ClearCacheOnPickle that you remove in your branch had several purposes: clear the cache on pickle but also on copy. Your version keep the cache on copy and that might not be appropriate for classes that used ClearCacheOnPickle.

Thanks for pointing this out. What should we do about this? Should caches ever be copied actually?

I guess not. And if you really want to, it is always possible to fill the cache manually.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 6 years ago by saraedum

Do you have any idea how I could implement that? I could for example implement __getstate__ for sage parents and elements, but then whenever it is overwritten I would have to rely on the super implementation being called. I could also implement __copy__ there which is muss less likely to be overwritten but still the problem is the same. The problem is that unlike with pickling, __getstate__ is not called on the cached methods when a shallow copy is made (only for a deep copy.) One thing I could think of is putting the id of the original object into each of its caches. Any call to the cache could check that id (which could be a small but significant performance loss) and complain if it does not match the element the cache is associated to.

I think that implementing __copy__ is probably the best path to take.

comment:23 in reply to: ↑ 22 Changed 6 years ago by nbruin

Replying to saraedum:

Do you have any idea how I could implement that? I could for example implement __getstate__ for sage parents and elements, but then whenever it is overwritten I would have to rely on the super implementation being called.

(I'm pretty sure that __getstate__ should call its super unless one really knows what one is doing. A bigger problem is that __getstate__ is only a convention that the standard __reduce__ uses. I'm sure there are classes that implement their own __reduce__ far below the point where you'd be thinking of injecting your __getstate__)

On #15207 there is some discussion of some possible infrastructure for managing these things. Perhaps it's relevant for your case?

I could also implement __copy__ there which is muss less likely to be overwritten but still the problem is the same.

I would recommend leaving __copy__ out of it. That has nothing to do with the pickling/serialization protocol. It provides a "cheap" way of replicating a mutable structure (note that for immutable structures, __copy__ routinely returns self, because immutability implies) so that mutations of one do not affect the other.

Mutable structures need to take care that they invalidate caches already (when mutated). I would say that a default behaviour where copy creates a new object with the same caches would be appropriate. The default behaviour of "copy" is to copy the instance dict, which largely establishes this.

If this is not appropriate behaviour, it seems to me that the class has rather special needs and hence should provide its own __copy__.

EDIT: I see that ClearCacheOnPickle gets its copy behaviour because under certain circumstances, copy(...) will revert to calling __getstate__. So I would expect that this would continue to be the case. Since the "clear cache on copy" is a bit of a side-effect of the implementation of ClearCacheOnPickle I would try and see if just changing the copy behaviour gives acceptable results. If not then providing a custom __copy__ on just the class that originally inherited from ClearCacheOnPickle would do the trick. Going forward, I think having caches replicated (or probably even shared!) between shallow copies by default doesn't seem like an unreasonable default.

Last edited 6 years ago by nbruin (previous) (diff)

comment:24 Changed 6 years ago by saraedum

Ok. Thanks for the input. I will implement it like that. Explicitly implement __copy__ where it is necessary to keep the old behaviour and leave everything else like it is in my branch.

comment:25 Changed 6 years ago by saraedum

  • Cc nthiery added

Trying to implement this, I realized that the point of the ClearCacheOnPickle on these classes was certainly not to clear *all* caches on copy. Also, ClearCacheOnPickle is broken, since it replaces any cache dictionary with an actual dict , i.e., it would replace a weak dictionary with a regular one.)

I guess there was in each case some cached method which should not be pickled (or probably copied.) So it is probably better to look at the actual occurences and fix the copying there instead of just hacking a "drop some caches" __copy__ method which probably does too much and might even break if we change the cached methods next time around.

The classes which carry ClearCacheOnPickle are:

  • ToricVariety_field where it was added in 0be2c23d with the comment "Do not pickle caches of toric varieties". So it could be alright if we still copied them.
  • WeilGroup_gens where it was added when the class did not provide the functionality to clear caches on copy. So it might be alright to copy them.
  • AmbientSpace and RootSpace where it is not clear why it was added exactly. Maybe Nicolas could clarify this (author of the commit which added it.)

Btw. the lengthy discussion in #11115 (this is where the clear caches on copy was introduced) does not make any mention of "copy".

comment:26 Changed 6 years ago by saraedum

  • Status changed from needs_info to needs_review

Since there has been no reaction for quite a while, I'm setting this back to needs review. I think my current implementation does the right thing.

comment:27 Changed 6 years ago by vdelecroix

  • Status changed from needs_review to needs_work

From the patchbot

sage -t --long src/sage/misc/cachefunc.pyx
**********************************************************************
File "src/sage/misc/cachefunc.pyx", line 3185, in sage.misc.cachefunc.cached_method
Failed example:
    id(d) == hash(d)
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of  30 in sage.misc.cachefunc.cached_method
    [830 tests, 1 failure, 15.75 s]
----------------------------------------------------------------------
sage -t --long src/sage/misc/cachefunc.pyx  # 1 doctest failed
----------------------------------------------------------------------

comment:28 Changed 6 years ago by saraedum

  • Status changed from needs_work to needs_review

That test does not fail for me. It is reported to be fine by another patchbot. Maybe the patchbot that ran it is broken somehow? I can not really imagine that something "random" is happening here.

comment:29 Changed 6 years ago by git

  • Commit changed from b2aa4073a6724e0c92c719f8d3eb232ef0bc2102 to 67c7782df66179ebc0e7dd09b2518f76b6ecf198

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

67c7782Merge branch 'develop' of git://trac.sagemath.org/sage into t/15692/ticket/15692

comment:30 follow-up: Changed 6 years ago by jdemeyer

What's the use case for pickle=True? I would say that a cache should never be pickled.

comment:31 in reply to: ↑ 30 ; follow-up: Changed 6 years ago by novoselt

Replying to jdemeyer:

What's the use case for pickle=True? I would say that a cache should never be pickled.

If something in cache is expensive to compute and you want to dump your object so that later you can load it with cache full. I use it regularly for my personal classes and some time ago it was also used for lists of reflexive polytopes (to store points and faces that were taking many seconds to recompute). One may argue whether it is the best way philosophically or not, but it is easy and it works, so it is nice to have this possibility available.

comment:32 in reply to: ↑ 31 Changed 6 years ago by jdemeyer

Replying to novoselt:

If something in cache is expensive to compute and you want to dump your object so that later you can load it with cache full. I use it regularly for my personal classes and some time ago it was also used for lists of reflexive polytopes (to store points and faces that were taking many seconds to recompute). One may argue whether it is the best way philosophically or not, but it is easy and it works, so it is nice to have this possibility available.

That seems like an argument for making pickle=True the default.

Something else: couldn't this be implemented more easily by a new class instead of adding pickle arguments everywhere?

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:33 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-6.9 to sage-7.0
  • Status changed from needs_review to needs_work

One easy simplification: make CacheDict never pickle its contents and just choose the type dict or CacheDict.

comment:34 Changed 6 years ago by jdemeyer

One easy simplification: make CacheDict never pickle its contents and just choose the type dict or CacheDict.

comment:35 Changed 6 years ago by jdemeyer

This is not grammatically correct:

Per default, the contents of the cache are not pickle::

comment:36 follow-up: Changed 6 years ago by jdemeyer

Can you replace the pickle keyword and attribute to do_pickle since that more accurately describes the intention.

Also, please explain the reason for changing both the pickling/unpickling of the cached function themselves as well as the CacheDict. It seems that they both solve your problem and I don't see why this problem needs to be solved twice.

comment:37 Changed 6 years ago by git

  • Commit changed from 67c7782df66179ebc0e7dd09b2518f76b6ecf198 to 54539bcd5bddd7038fb5b0fd46a992c6ff96d238

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

54539bcMerge branch 'develop' into t/15692/ticket/15692

comment:38 Changed 6 years ago by git

  • Commit changed from 54539bcd5bddd7038fb5b0fd46a992c6ff96d238 to 505c7af667df54ee28441a74da0896fe358ddb31

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c182f40Remove ClearCacheOnPickle meta class
505c7afDo not pickle cache methods

comment:39 Changed 6 years ago by git

  • Commit changed from 505c7af667df54ee28441a74da0896fe358ddb31 to e3a7eb4e7ec0852fb058e5455d270067846ea2b6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e3a7eb4Do not pickle cached methods

comment:40 in reply to: ↑ 36 Changed 6 years ago by saraedum

Replying to jdemeyer:

Can you replace the pickle keyword and attribute to do_pickle since that more accurately describes the intention.

Fixed.

Also, please explain the reason for changing both the pickling/unpickling of the cached function themselves as well as the CacheDict. It seems that they both solve your problem and I don't see why this problem needs to be solved twice.

You are right. I did look at the (do_)pickle parameter in too many places.

comment:41 Changed 6 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:42 Changed 6 years ago by saraedum

  • Keywords days71 added

comment:43 Changed 6 years ago by git

  • Commit changed from e3a7eb4e7ec0852fb058e5455d270067846ea2b6 to 746dc53e1d767870430a3770eef1159c6534b9b1

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

746dc53fix grammar in doctest

comment:44 Changed 6 years ago by roed

  • Reviewers changed from Vincent Delecroix to Vincent Delecroix, David Roe
  • Status changed from needs_review to positive_review

Looks good to me.

comment:45 Changed 6 years ago by aly.deines

I'm not going to change this from positive review, but I found the tiniest doctest typo:

+        Per default, the contents of the cache are not pickle::

pickle should be pickled here.

comment:46 Changed 6 years ago by roed

I think you're looking at an old version; that typo was what was fixed in 746dc53.

comment:47 Changed 6 years ago by aly.deines

Awesome!

comment:48 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

Fails on 32-bit:

sage -t --long src/sage/misc/cachefunc.pyx
**********************************************************************
File "src/sage/misc/cachefunc.pyx", line 3161, in sage.misc.cachefunc.cached_method
Failed example:
    id(d) == hash(d)
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of  30 in sage.misc.cachefunc.cached_method
    [843 tests, 1 failure, 8.96 s]

comment:49 Changed 5 years ago by git

  • Commit changed from 746dc53e1d767870430a3770eef1159c6534b9b1 to 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4

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

3a6067aMerge branch 'develop' of https://github.com/sagemath/sage into t/15692/ticket/15692
7ef7a41Remove id check from cached method doctest

comment:50 Changed 5 years ago by saraedum

  • Status changed from needs_work to positive_review

comment:51 Changed 5 years ago by saraedum

Since this is such a minor change, I set this back to positive review.

comment:52 Changed 5 years ago by fbissey

I suspect I am getting this in sage-on-gentoo pulling volker's current develop branch because of this:

File "/usr/lib64/python2.7/site-packages/sage/modular/overconvergent/weightspace.py", line 473, in sage.modular.overconvergent.weightspace.AlgebraicWeight
Failed example:
    w == loads(dumps(w))
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 501, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 864, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.modular.overconvergent.weightspace.AlgebraicWeight[3]>", line 1, in <module>
        w == loads(dumps(w))
      File "sage/structure/element.pyx", line 913, in sage.structure.element.Element.__richcmp__ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/structure/element.c:9542)
        return (<Element>self)._richcmp_(<Element>other, op)
      File "sage/structure/element.pyx", line 948, in sage.structure.element.Element._richcmp_ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/structure/element.c:9722)
        c = left._cmp_(right)
      File "sage/structure/element.pyx", line 964, in sage.structure.element.Element._cmp_ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/structure/element.c:10101)
        return left_cmp(right)
      File "/usr/lib64/python2.7/site-packages/sage/modular/overconvergent/weightspace.py", line 417, in __cmp__
        return cmp(self.values_on_gens(), other.values_on_gens())
      File "/usr/lib64/python2.7/site-packages/sage/modular/overconvergent/weightspace.py", line 380, in values_on_gens
        return ( self(self.parent()._param), self.teichmuller_type())
      File "/usr/lib64/python2.7/site-packages/sage/modular/overconvergent/weightspace.py", line 539, in __call__
        if self._p**(x.precision_absolute()) < self._chi.conductor():
      File "sage/misc/cachefunc.pyx", line 1717, in sage.misc.cachefunc.CachedMethodPickle.__call__ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/misc/cachefunc.c:8703)
        return CM(*args,**kwds)
      File "sage/misc/cachefunc.pyx", line 2401, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/misc/cachefunc.c:12708)
        self.cache = f(self._instance)
      File "/usr/lib64/python2.7/site-packages/sage/modular/dirichlet.py", line 755, in conductor
        if self.modulus() == 1 or self.is_trivial():
      File "sage/misc/cachefunc.pyx", line 1717, in sage.misc.cachefunc.CachedMethodPickle.__call__ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/misc/cachefunc.c:8703)
        return CM(*args,**kwds)
      File "sage/misc/cachefunc.pyx", line 2401, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/misc/cachefunc.c:12708)
        self.cache = f(self._instance)

and ending after many lines with

      File "sage/misc/cachefunc.pyx", line 2401, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/misc/cachefunc.c:12708)
        self.cache = f(self._instance)
      File "/usr/lib64/python2.7/site-packages/sage/modular/dirichlet.py", line 1738, in element
        v = M([dlog[x] for x in self.values_on_gens()])
      File "/usr/lib64/python2.7/site-packages/sage/modular/dirichlet.py", line 1705, in values_on_gens
        v = tuple([pows[i] for i in self.element()])
      File "sage/misc/cachefunc.pyx", line 2401, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/misc/cachefunc.c:12708)
        self.cache = f(self._instance)
      File "/usr/lib64/python2.7/site-packages/sage/modular/dirichlet.py", line 1730, in element
        M = P._module
      File "/usr/lib64/python2.7/site-packages/sage/modular/dirichlet.py", line 2137, in _module
        len(self.unit_gens()))
      File "sage/structure/factory.pyx", line 361, in sage.structure.factory.UniqueFactory.__call__ (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/structure/factory.c:1585)
        key, kwds = self.create_key_and_extra_args(*args, **kwds)
      File "sage/structure/factory.pyx", line 459, in sage.structure.factory.UniqueFactory.create_key_and_extra_args (/scratch2/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/structure/factory.c:2722)
        return self.create_key(*args, **kwds), {}
      File "/usr/lib64/python2.7/site-packages/sage/modules/free_module.py", line 336, in create_key
        rank = int(sage.rings.integer.Integer(rank))
    RuntimeError: maximum recursion depth exceeded while calling a Python object
**********************************************************************
1 item had failures:
   2 of   6 in sage.modular.overconvergent.weightspace.AlgebraicWeight
    [108 tests, 2 failures, 0.32 s]
----------------------------------------------------------------------
sage -t --long /usr/lib64/python2.7/site-packages/sage/modular/overconvergent/weightspace.py  # 2 doctests failed
----------------------------------------------------------------------

There are two of those. There is always the possibility that I missed a patch in python or some other place that deals with this but I don't think so.

comment:53 Changed 5 years ago by vbraun

  • Branch changed from u/saraedum/ticket/15692 to 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:54 Changed 5 years ago by vbraun

  • Commit 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

comment:55 Changed 5 years ago by vbraun

Indeed the two doctest failures

comment:56 Changed 5 years ago by fbissey

Actually the main failure is in sage/modular/dirichlet.py, the other sage/modular/overconvergent/weightspace.py fails because it is calling dirichlet. So one only needs to fix dirichlet.py.

comment:57 Changed 5 years ago by saraedum

  • Branch changed from 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4 to u/saraedum/ticket/15692
  • Commit set to 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4

New commits:

c182f40Remove ClearCacheOnPickle meta class
e3a7eb4Do not pickle cached methods
746dc53fix grammar in doctest
3a6067aMerge branch 'develop' of https://github.com/sagemath/sage into t/15692/ticket/15692
7ef7a41Remove id check from cached method doctest

comment:58 Changed 5 years ago by git

  • Commit changed from 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4 to a152dc4a2d7f07841152421bde45c003b6d2b6f1

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

d2da8dcremove unused do_pickle field from CachedMethod
6c36386Merge branch 'u/saraedum/ticket/15692' of git://trac.sagemath.org/sage into t/15692/ticket/15692
125ec7fMerge branch 'develop' of https://github.com/sagemath/sage into t/15692/ticket/15692
aaebb01Merge branch 'develop' into t/15692/ticket/15692
a152dc4Pickle caches of dirichlet characters

comment:59 Changed 5 years ago by saraedum

  • Status changed from new to needs_review

The dirichlet.py doctests pass now. Let's see what the patchbot thinks about the rest.

comment:60 Changed 5 years ago by saraedum

  • Status changed from needs_review to needs_work
  • Work issues set to pickle jar

comment:61 Changed 5 years ago by git

  • Commit changed from a152dc4a2d7f07841152421bde45c003b6d2b6f1 to 141af2efcb44d8048fed779d2b719d20a243c31c

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

141af2eUnpickle legacy dirichlet characters

comment:62 Changed 5 years ago by saraedum

  • Status changed from needs_work to needs_review
  • Work issues pickle jar deleted

comment:63 Changed 5 years ago by fbissey

  • Status changed from needs_review to needs_work

Looks like your last commit needs a tiny little bit more work

sage -t --long src/sage/modular/dirichlet.py
**********************************************************************
File "src/sage/modular/dirichlet.py", line 1760, in sage.modular.dirichlet.DirichletCharacter.__setstate__
Failed example:
    loads(dumps(e)) == e
Expected nothing
Got:
    True

From the bot.

comment:64 Changed 5 years ago by git

  • Commit changed from 141af2efcb44d8048fed779d2b719d20a243c31c to a18cbf54adcc6684fdd9206fa837fe07a2942215

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

a18cbf5Fixed doctest for element of DirichletGroup

comment:65 Changed 5 years ago by saraedum

Sorry, that was dumb. I hope it's better now :)


New commits:

a18cbf5Fixed doctest for element of DirichletGroup

comment:66 Changed 5 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:67 Changed 5 years ago by fbissey

  • Status changed from needs_review to positive_review

Bot happy, I am putting this back to positive.

comment:68 Changed 5 years ago by vbraun

  • Branch changed from u/saraedum/ticket/15692 to a18cbf54adcc6684fdd9206fa837fe07a2942215
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:69 Changed 5 years ago by jdemeyer

  • Authors changed from Julian Rueth to Julian Rüth
  • Commit a18cbf54adcc6684fdd9206fa837fe07a2942215 deleted
Note: See TracTickets for help on using tickets.