#218 Make everything picklable #655

rmjarvis · 2015-03-21T08:11:59Z

This is a really old issue that's been kind of irksome that we haven't gotten around to yet. So a few weeks ago, I decided to tackle it. I'm not actually 100% done yet, but it's close enough that I think it's worth having people take a look.

Almost all of our classes now have the following features:

Picklable. i.e. cPickle.loads(cPickle.dumps(obj)) gets you back an exact copy of the original
Copyable. i.e. copy.copy(obj) makes a copy of the object. Anything with an rng attribute won't be identical, since the RNG is just copied, not duplicated.
Deep copyable. i.e. copy.deepcopy(obj) makes an exact copy of the original. Things with an rng attribute are identical in this case.
Repr-able. i.e. eval(repr(obj)) makes an exact copy of the original. The usual recommendation for repr is that it be "unambiguous". A common way to make sure of that is to have the repr string be eval-able, at least in some context (e.g. after import galsim). However, in our case, this means the repr string in some cases is rather long, so I'm quite willing to reconsider whether this is a feature we really want.
String-able. i.e. str(obj) returns a reasonable looking string that describes the object.

Probably most people won't want to read through the code changes I made to make all this work, but I would encourage people to play around with printing str(obj) and repr(obj) to see how things look.

Conflicts: galsim/__init__.py galsim/image.py include/galsim/Random.h pysrc/Image.cpp pysrc/Random.cpp

(#218)

…ue using test-based design... :) (#218)

(#218)

… BaseNoise not construcable. (Too hard to pickle.) (#218)

(#218)

Conflicts: galsim/correlatednoise.py

Conflicts: galsim/correlatednoise.py galsim/interpolatedimage.py galsim/noise.py pysrc/Noise.cpp pysrc/Random.cpp pysrc/SBProfile.cpp tests/test_correlatednoise.py

(#218)

rmandelb · 2015-04-16T18:16:38Z

galsim/chromatic.py

+            self.original = obj
+            self._jac = jac
+            self._off = offset
+            self._frat = flux_ratio


Any reason not to call this self._flux_ratio? While skimming the code I didn't notice this line and only saw _frat later on, and wondered what it was (seems non-obvious to me).

Just brevity. For private (implementation-detail) attributes, I don't feel as obliged to be verbose. But I can change it.

(#218)

rmandelb · 2015-04-16T18:35:53Z

galsim/nfw_halo.py

@@ -42,7 +42,15 @@ def __init__(self, omega_m=0.3, omega_lam=0.7):
        self.omega_lam = omega_lam
        self.omega_c = (1. - omega_m - omega_lam)
        #self.omega_r = 0


This has nothing to do with this PR, but if we are ignoring radiation in most of the code, is there any reason to keep the two commented-out lines that involve omega_r?

I assume they were put in as a starter implementation in case we eventually wanted to enable radiation. I don't think it hurts to leave them here.

There are other places that would also have to change, but don't have comments. So it seemed a bit inconsistent to me. But I guess it doesn't matter, we can cross that bridge when/if we come to it. :)

rmandelb · 2015-04-16T18:46:08Z

A random thought that occurred to me as I was going through this (not done yet):
It would be nice to show the use of __str__ and __eq__ for some of the objects for which this functionality is newly enabled in a demo or two.

(#218)

rmandelb · 2015-04-16T18:58:55Z

galsim/shear.py

+        return self._g.imag * self._g2e(abs(self._g)**2)
+
+    def getE(self):
+        """Return the magnitude of the distortion |e1 + i e2| = sqrt(e1**2 + e2**2)


We seem to go back and forth in these docstrings between i and j for sqrt(-1). Should probably choose just one and stick with it.

I think the reason is that for getShear() I thought it made more sense to use the python version of i, which is j. So I wanted to point out that getShear() was g1 + 1j * g2, which is how you would build the complex object in python. Then that idea carried over into some others like getG and getBeta.

But for these, it's probably better to just use i (like I did here), since they are all just describing the math, not python, and i is the more normal thing to use for that.

(#218)

rmandelb · 2015-04-16T19:57:00Z

So if we are being serious about having immutable types, then it should really return a different object.

OK.

(#218)

…out python complex type (#218)

(#218)

rmjarvis · 2015-04-17T03:30:39Z

A random thought that occurred to me as I was going through this (not done yet):
It would be nice to show the use of str and eq for some of the objects for which this functionality is newly enabled in a demo or two.

I couldn't think of any place to use __eq__, but demo7 seems like a good place to use the str functionality. See what you think about my latest commit: 9248b0d.

(#218)

rmandelb · 2015-04-17T12:25:49Z

See what you think about my latest commit:

Looks good to me.

Also, in case it wasn't clear, I did finish going over the diff yesterday and am happy for this to be merged once you're done with my comments (which I think may be the case already, based on the commits above that I glanced through).

rmjarvis · 2015-04-17T12:31:44Z

Great. I did address all the other comments you had yesterday. Thanks for those!

I'll plan on merging tonight, and then do #644 on master. Then we can work on releasing v1.3.

rmandelb · 2015-04-17T12:36:04Z

Sounds good to me.

#218 Make everything picklable

TallJimbo and others added 30 commits July 19, 2012 15:45

Started pickle/copy support with Angle, Position, Box classes.

a9bdd04

Replaced pickle support for images so copy operations work properly too.

f2f0f3f

Pickle/copy support for random deviates, Shear, PhotonArray.

780c258

Merge branch 'master' into #218

ba10e25

Conflicts: galsim/__init__.py galsim/image.py include/galsim/Random.h pysrc/Image.cpp pysrc/Random.cpp

Add back test_image.py

9c42221

(#218)

Get Image class back to existing serialization method

36e758c

(#218)

Remove Jim's serialization for BaseDeviate

054cd54

(#218)

Remove Jim's unit test for picklability

1e6ecbc

(#218)

Remove the file Jim had for doing pickle support

4c4d025

(#218)

Add unit tests of picklability. Most fail currently. I'll do this iss…

180f68e

…ue using test-based design... :) (#218)

Add unit tests for Angle and AngleUnit class

a2cd4f4

(#218)

Add Image == operator

d192bde

(#218)

Simplify existing __getinitargs__ functions

e2ef1ca

(#218)

Move longer GSParams functions to GSParams.cpp

8318e01

(#218)

Enable pickling of basic GSObjects

0381349

(#218)

Enable pickling of Noise classes

8fbd22e

(#218)

Disconnect BaseCorrelatedNoise from BaseNoise base class since I made…

f5a3d4d

… BaseNoise not construcable. (Too hard to pickle.) (#218)

Add repr, str for Deviate and Noise classes

cc0387b

(#218)

Improve the Angle and CelestialCoord str, repr functions

13d6c1a

(#218)

Improve the str and repr of basic GSObjects

b3fc5b2

(#218)

Merge branch 'master' into #218

d2cd2f5

Conflicts: galsim/correlatednoise.py

Merge branch '#643' into #218

2611a97

Conflicts: galsim/correlatednoise.py galsim/interpolatedimage.py galsim/noise.py pysrc/Noise.cpp pysrc/Random.cpp pysrc/SBProfile.cpp tests/test_correlatednoise.py

Fix errors from merge

7a63d88

(#218)

Make Shapelet picklable

f2910a2

(#218)

Add more tests of Shear in preparation for reimplementing it in python

42a59b1

(#218)

Reimplement Shear class in python

37cca28

(#218)

Expunge CppShear from repo

0619178

(#218)

Enable pickling for all WCS classes

da10d3b

(#218)

Fix repr for Image objects

e124824

(#218)

Enable pickling for Bandpass objects

4503f8a

(#218)

rmandelb reviewed Apr 16, 2015
View reviewed changes

rmandelb added 3 commits April 16, 2015 14:24

OCD sentence fragment edit

a5445e0

(#218)

typo

c0540a0

(#218)

OCD sentence fragment edit (again)

efaba01

(#218)

rmandelb reviewed Apr 16, 2015
View reviewed changes

rmandelb added 3 commits April 16, 2015 14:47

typo

d669484

(#218)

spacing conventions

f0b127f

(#218)

typo

1cc9b3a

(#218)

rmandelb reviewed Apr 16, 2015
View reviewed changes

rmandelb added 3 commits April 16, 2015 15:03

typo

cc1bda7

(#218)

punctuation

65affda

(#218)

typo

838894e

(#218)

rmjarvis added 8 commits April 16, 2015 17:35

Address Rachel's comments re Bandpass

da658cc

(#218)

Change name of setupInterpolation to interpolate

209c850

(#218)

Use more verbose (and clearer) names _offset and _flux_ratio

08d919c

(#218)

Add caveat that obj.copy() is a shallow copy.

218b566

(#218)

typo

d2a69d9

(#218)

Use i in Shear docs rather than 1j except where explicitly talking ab…

c672b6a

…out python complex type (#218)

Make many of the GSObject str representations a bit simpler

5612b4e

(#218)

Update demo7 to use str(obj) in output logger messages

9248b0d

(#218)

typo

eef6d12

(#218)

rmjarvis added a commit that referenced this pull request Apr 17, 2015

Merge pull request #655 from GalSim-developers/#218

7315168

#218 Make everything picklable

rmjarvis merged commit 7315168 into master Apr 17, 2015

rmjarvis deleted the #218 branch April 17, 2015 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#218 Make everything picklable #655

#218 Make everything picklable #655

rmjarvis commented Mar 21, 2015

rmandelb Apr 16, 2015

rmjarvis Apr 16, 2015

rmandelb Apr 16, 2015

rmjarvis Apr 16, 2015

rmandelb Apr 16, 2015

rmandelb commented Apr 16, 2015

rmandelb Apr 16, 2015

rmjarvis Apr 16, 2015

rmandelb commented Apr 16, 2015

rmjarvis commented Apr 17, 2015

rmandelb commented Apr 17, 2015

rmjarvis commented Apr 17, 2015

rmandelb commented Apr 17, 2015

#218 Make everything picklable #655

#218 Make everything picklable #655

Conversation

rmjarvis commented Mar 21, 2015

rmandelb Apr 16, 2015

Choose a reason for hiding this comment

rmjarvis Apr 16, 2015

Choose a reason for hiding this comment

rmandelb Apr 16, 2015

Choose a reason for hiding this comment

rmjarvis Apr 16, 2015

Choose a reason for hiding this comment

rmandelb Apr 16, 2015

Choose a reason for hiding this comment

rmandelb commented Apr 16, 2015

rmandelb Apr 16, 2015

Choose a reason for hiding this comment

rmjarvis Apr 16, 2015

Choose a reason for hiding this comment

rmandelb commented Apr 16, 2015

rmjarvis commented Apr 17, 2015

rmandelb commented Apr 17, 2015

rmjarvis commented Apr 17, 2015

rmandelb commented Apr 17, 2015