Handle numpy pure scalar types#62
Conversation
spawned from enthought/mayavi#61 The state pickler/unpickler could not handle numpy scalar types, which were forced to None. This meant that trait classes having Float() trait hosting a numpy.float64 could not be pickled (to be precise they were pickled, with the content being None, silently).
|
Failures were already in master and are due to 2.6 and 3.4 incompatibilities. |
|
@stefanoborini I think we can start dropping the 2.6 support in apptools |
|
@dpinte ok. |
|
Strangely enough, tests pass on my machine with python 3.4. |
| # want to recover their true type, and the only way of doing so | ||
| # is to consider them as objects. | ||
| if isinstance(value, numpy.generic): | ||
| return id(value) |
There was a problem hiding this comment.
There is a small problem with using id here. The same id can be re-used when the original object is garbage collected. So if during the pickling process the numpy arrays are garbage collected then the registry might point to the wrong object.
There was a problem hiding this comment.
I have updated the pickling _do_... method to keep the object alive in the chase (for the duration of the pickling process). This should be enough to avoid the id reuse case.
There was a problem hiding this comment.
Is there a simple way to avoid the caching altogether? The current caching code is broken (#66), and I don't think it makes a lot of sense to be caching simple immutable scalars in the first place: the size of the "reference" dictionary generated is going to be comparable to the size of the object dictionary.
| def _do_numpy_generic_type(self, value): | ||
| idx = self._register(value) | ||
| self._misc_cache.append(value) | ||
| data = base64_encode(pickle.dumps(value)) |
There was a problem hiding this comment.
Pickling numpy scalars is quite costly (in space). I wonder if there is a better way.
There was a problem hiding this comment.
But otherwise, not so much. You'll end up reproducing exactly what pickle does anyways, if you want to be general.
There was a problem hiding this comment.
data = base64_encode(value.dumps())
Current coverage is 80.36%@@ master #62 diff @@
========================================
Files 108 108
Lines 4586 4609 +23
Methods 0 0
Branches 732 712 -20
========================================
+ Hits 3683 3704 +21
- Misses 744 757 +13
+ Partials 159 148 -11
|
|
@prabhuramachandran can you have a look. is it ok to merge this PR? |
|
Imho, it's good for me. Good spot. |
| return id(value) | ||
|
|
||
| try: | ||
| key = hash(value) |
There was a problem hiding this comment.
The use of hash(value) as a key is broken: it's assuming that hash will be one-to-one. This is orthogonal to the issue fixed by this PR; I'll open a separate issue.
There was a problem hiding this comment.
I'll open a separate issue.
6 participants
spawned from enthought/mayavi#61
The state pickler/unpickler could not handle numpy scalar types,
which were forced to None. This meant that trait classes having
Float() trait hosting a numpy.float64 could not be pickled
(to be precise they were pickled, with the content being None, silently).