Value of cached methods should not always be pickled

[simon-king-jena]

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

Depends on #16337

CC: @nathanncohen @vbraun @nbruin @novoselt @sagetrac-jkeitel @nthiery

Component: pickling

Keywords: hash, cache, days71

Author: Julian Rüth

Branch: a18cbf5

Reviewer: Vincent Delecroix, David Roe

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

[saraedum]

comment:3

In #15149 it was proposed that caches should be 'opt-in and not opt-out' (#15149 comment:6). 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, i.e., the default for a cache would be that it is not pickled?

[saraedum]

Branch: u/saraedum/ticket/15692

[saraedum]

Dependencies: #16337

[saraedum]

Author: Julian Rueth

[novoselt]

comment:7

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

---

New commits:

ce27b84	Removed sage.misc.cachefunc.ClearCacheOnPickle
e27e316	Propagate key of a @cached_method correctly
94fcddb	Merge branch 'u/saraedum/ticket/16337' of git://trac.sagemath.org/sage into ticket/15692
72fce8b	Added a pickle parameter for @cached_method
0717849	Enable pickling of the cache for groebner_basis()

[novoselt]

Commit: 0717849

[sagetrac-git]

Changed commit from 0717849 to 9eab84b

[sagetrac-git]

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

9eab84b

Merge branch 'develop' into ticket/15692

[fchapoton]

comment:10

does not apply, needs rebase

[sagetrac-git]

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

13b4e4c

Merge branch 'develop' into t/15692/ticket/15692

[sagetrac-git]

Changed commit from 9eab84b to 13b4e4c

[fchapoton]

comment:13

one failing doctest, see patchbot report

[saraedum]

comment:59

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

[saraedum]

Work Issues: pickle jar

[sagetrac-git]

Changed commit from a152dc4 to 141af2e

[sagetrac-git]

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

141af2e

Unpickle legacy dirichlet characters

[saraedum]

Changed work issues from pickle jar to none

[kiwifb]

comment:63

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.

[sagetrac-git]

Changed commit from 141af2e to a18cbf5

[sagetrac-git]

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

a18cbf5

Fixed doctest for element of DirichletGroup

[saraedum]

comment:65

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

---

New commits:

a18cbf5

Fixed doctest for element of DirichletGroup

[kiwifb]

comment:67

Bot happy, I am putting this back to positive.

[vbraun]

Changed branch from u/saraedum/ticket/15692 to a18cbf5

[jdemeyer]

Changed commit from a18cbf5 to none

[jdemeyer]

Changed author from Julian Rueth to Julian Rüth