Cached methods with do_pickle=True fail for UniqueRepresentation objects

[kwankyu]

The decorator @cached_method(do_pickle=True) is used to make a method to cache the result and the cached result pickled along with the object.

This fails for objects of UniqueRepresentation class. It is a consequence of the defective behavior of UniqueRepresentation object not putting its state into the pickle.

This ticket solves the problem. A discussion on this issue was in

https://groups.google.com/forum/#!topic/sage-devel/n7v71cnVTmk

CC: @simon-king-jena @tscrim @nbruin

Component: pickling

Author: Kwankyu Lee

Branch/Commit: u/mkoeppe/28096 @ ab2eb86

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

[kwankyu]

Branch: u/klee/28096

[kwankyu]

Author: Kwankyu Lee

[sagetrac-git]

Commit: f17e57a

[sagetrac-git]

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

f17e57a

Make UniqueRepresentation do_pickle

[nbruin]

comment:4

We can definitely not turn pickling on by default. See #15692.

[kwankyu]

comment:5

This is not about turn pickling on by default. This turns on pickling only for @cached_methods with do_pickle=True.

[kwankyu]

comment:6

This ticket makes UniqueRepresentation objects do what other sage objects do, for pickling cached methods.

[sagetrac-git]

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

09fc937

Ignore cached functions with do_pickle=False

[sagetrac-git]

Changed commit from f17e57a to 09fc937

[kwankyu]

comment:8

To a reviewer:

Before this ticket,

• the cached function attached to cached method with do_pickle=False (the default) is not pickled

• the cached function attached to cached method with do_pickle=True is not pickled

With this ticket and before the last commit,

• the cached function attached to cached method with do_pickle=False (the default) is pickled without the cache

• the cached function attached to cached method with do_pickle=True is pickled with the cache

After the last commit,

• the cached function attached to cached method with do_pickle=False (the default) is not pickled

• the cached function attached to cached method with do_pickle=True is pickled with the cache

This last pickling behaviour of UniqueRepresentation objects is somewhat halfway between the old pickling behavior of UniqueRepresentation objects and that of other sage objects (second behavior).

The last version seems preferable because it does not increase the size of a pickle to keep unnecessary information, namely cached function without the cache.

[sagetrac-git]

Changed commit from 09fc937 to c979635

[sagetrac-git]

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

c979635

Fix a format mistake

[sagetrac-git]

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

983577b	Merge branch 'develop'
9d6e0c7	Rename to is_pickled_with_cache

[sagetrac-git]

Changed commit from c979635 to 9d6e0c7

[sagetrac-git]

Changed commit from 9d6e0c7 to 6d47b68

[sagetrac-git]

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

6d47b68

A little more documentation fixes

[simon-king-jena]

comment:14

After a brief look at the diff, it seems to me that there are many changes that have nothing to do with the purpose of this ticket: You remove from future import absolute_import and you have various changes of the following form:

@@ -167,9 +167,9 @@ class Manifolds(Category_over_base_ring):
 
             TESTS::
 
-                sage: TestSuite(Manifolds(RR).Smooth()).run()
                 sage: Manifolds(RR).Smooth.__module__
                 'sage.categories.manifolds'
+                sage: TestSuite(Manifolds(RR).Smooth()).run()
             """
             return self._with_axiom('Smooth')

Why?

[kwankyu]

comment:15

Replying to @simon-king-jena:

After a brief look at the diff, it seems to me that there are many changes that have nothing to do with the purpose of this ticket: You remove from future import absolute_import

Right. This is nothing to do with the purpose of the ticket. You know that this import statement is not necessary any more since python 2.7. It happens that as I am writing a patch for a ticket, I encounter things to be easily fixed, like obvious typos, docstrings in bad style, unnecessary imports (like the above) and so on. Sometimes I fix them. Other times I pass them.

Is this bad? Do you think I should revert these small changes not related with the purpose of the ticket before I push the commits?

I think if developers do not make such spontaneous fixes, improvement of the overall quality of sage code will get slower.

and you have various changes of the following form:

@@ -167,9 +167,9 @@ class Manifolds(Category_over_base_ring):
 
             TESTS::
 
-                sage: TestSuite(Manifolds(RR).Smooth()).run()
                 sage: Manifolds(RR).Smooth.__module__
                 'sage.categories.manifolds'
+                sage: TestSuite(Manifolds(RR).Smooth()).run()
             """
             return self._with_axiom('Smooth')

Why?

This (and many others like this) was related with the ticket. Without this reordering, I got doctest failures. But strangely but happily, now with 8.9.beta5, I don't have them any more even without reordering. I will revert back these changes.

[sagetrac-git]

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

c14d10b	Merge branch 'develop'
bd0bebb	Merge branch 'develop'
bcd500e	Revert back unnecessary changes in doctests

[sagetrac-git]

Changed commit from 6d47b68 to bcd500e

[mwageringel]

comment:17

Replying to @kwankyu:

Replying to @simon-king-jena:

After a brief look at the diff, it seems to me that there are many changes that have nothing to do with the purpose of this ticket: You remove from future import absolute_import

Right. This is nothing to do with the purpose of the ticket. You know that this import statement is not necessary any more since python 2.7.

I might be wrong, but I think this is still needed in Python 2.7, as I have recently had to add the statement in one of my own projects to solve a problem that did not exist with Python 3.

[sagetrac-git]

Changed commit from bcd500e to 084a2ab

[sagetrac-git]

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

084a2ab

Add back absolute_import

[kwankyu]

comment:19

Replying to @mwageringel:

Replying to @kwankyu:

Replying to @simon-king-jena:

After a brief look at the diff, it seems to me that there are many changes that have nothing to do with the purpose of this ticket: You remove from future import absolute_import

Right. This is nothing to do with the purpose of the ticket. You know that this import statement is not necessary any more since python 2.7.

I might be wrong, but I think this is still needed in Python 2.7, as I have recently had to add the statement in one of my own projects to solve a problem that did not exist with Python 3.

Ah, I am wrong. You are right. I had somehow wrong understanding on this matter. I added back the imports.

Thanks!

[mkoeppe]

comment:39

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:40

Setting a new milestone for this ticket based on a cursory review.

[mkoeppe]

comment:41

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

[mkoeppe]

comment:42

needs rebase

[sagetrac-git]

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

6f14c1c

Make do_pickle work for UniqueRepresentation objects

[sagetrac-git]

Changed commit from f0b387e to 6f14c1c

[mkoeppe]

Changed branch from u/klee/28096 to u/mkoeppe/28096

[mkoeppe]

Changed commit from 6f14c1c to ab2eb86

[mkoeppe]

comment:47

rebased

---

New commits:

ab2eb86

Make do_pickle work for UniqueRepresentation objects

[kwankyu]

comment:48

Thank you. This is long neglected, even by me. Perhaps it may need some revision.

[mkoeppe]

comment:49

I ran into an issue with UniqueRepresentation in #19195, so I'm taking a look at open related tickets (#25388, #25389, #14167)