Make GSObjects immutable?

[rmjarvis]

This is a proposal. I welcome comments...

As I've been learning python over the past two years (mostly in the course of working on GalSim), I've come to realize that it works much better with immutable classes. This is mostly because of the way the python = operator works, being a naming operator rather than assignment. This is why a number of places in GalSim we have to be careful about not modifying the inputs to functions, since we recognize that this behavior would be surprising to the python user who usually expects functions to not modify the inputs.

I've managed to get a number of classes in GalSim to be immutable. The new WCS classes are all immutable. Josh's Bandpass and SED are immutable. I recently changed Angle and BoundsI/D to be immutable (in the python layer, not in C++).

The usual syntax when you want to "change" one of these immutable objects is wcs = wcs.setOrigin(origin) or theta = theta.wrap(). This reassigns the new value to the original name. In fact, this is what python already does for the mutating operators like a += b and the like. It is really just shorthand for a = a + b. i.e. the original a is preserved if anyone else references it, and locally the quantity a + b is given the name a. As such, I have also been moving away from defining these operators in GalSim, letting python use the default definitions, since this will usually behave the way users will expect in python.

However, GSObjects are not currently immutable in GalSim. In particular, we have functions like applyShear, setFlux, etc. which do change the object rather than return a new object. In retrospect, knowing what I know now about how python works, I would have preferred that we had made these functions return new objects that are appropriately modified versions of the original. i.e. the way the various create methods work: createSheared and the like. This would have avoided a number of difficulties we have since had with respect to GSObjects, most notably how hard it is to make them picklable.

So my (somewhat radical) proposal is to switch GSObject and ChromaticObject to an immutable design where we remove or deprecate the mutating methods and switch over to using the form gal = gal.createSheared(shear) in all examples and documentation. In some cases, we'll need to make a new function if the create version is not available, such as setFlux.

The other half of my proposal is to revisit the names for these functions. The create names are a little stilted sounding, IMO. It wasn't really a big deal before, since the apply methods were available, which I thought read nicely, but I've never particularly liked the create function names.

Unfortunately, I think it is probably unwise to just use the apply names for a new version that returns the changed object, even though gal = gal.applyShear(shear) reads pretty nicely. I think it is better to come up with new names to make sure we can raise DeprecationWarning messages for the old forms.

Proposed names:

obj.applyShear(shear) -> obj = obj.shear(shear)
obj.applyRotation(theta) -> obj = obj.rotate(theta)
obj.applyExpansion(scale) -> obj = obj.expand(scale)
obj.applyDilation(scale) -> obj = obj.dilate(scale)
obj.applyMagnification(scale) -> obj = obj.magnify(mu)
obj.applyShift(dx,dy) -> obj = obj.shift(dx,dy)
obj.applyTransformation(dudx,...) -> obj = obj.transform(dudx,...)
obj.scaleFlux(fluxRatio) -> obj = obj * fluxRatio  # Already works this way!
obj.applyLensing(g1,g2,mu) -> obj = obj.lensing(g1,g2,mu)  # ???  I don't like this one.
obj.setFlux(flux) -> obj = obj.setFluxTo(flux)  # ???  Unfortunate, imo, but acceptable.

In most cases, the shorter verb form seems to make sense, applyLensing and setFlux being the only two that don't work so nicely, imo. I am (very!) open to suggestions and comments about any of this proposal.

If people agree with the proposal, it would be nice to get this done sooner than later since it is a big API change, and the sooner we switch, the less painful it will be, because our user base will only increase (hopefully!).

[msimet]

This sounds reasonable to me as a design choice, and, in fact, a little more intuitive. I wonder how common the mutable versions are in current user code though.

Name choice: obj.applyLensing(...) could just become obj.lens[e](...) (I always thought "to lense" was the proper spelling for the verb, but Google doesn't agree with me and now I am uncertain).

[gbernstein]

I won't comment about the usability issues but what are the performance impacts: if every object that gets drawn needs to be modified several times from its atomic type, and each modification now requires a copy operation and creation/destruction of another object, is this a significant slowdown? In particular I'd worry about the pixelated objects. The analytic objects should not have much of an issue. Can you do some tests of whether the shearing/shifting/scaling/reconvolving of a real galaxy image would take significant time compared to the FFT to render it at the end?

[rmjarvis]

I always thought "to lense" was the proper spelling for the verb, but Google doesn't agree with me and now I am uncertain.

Adding the e would normally make the s into an [s] sound rather than a [z] sound. e.g. rinse, tense, sense, response, dense. I think the only exception might be cleanse, which is probably why your brain likes the e on lense. It rhymes, so similar neural pathways. :)

[rmjarvis]

if every object that gets drawn needs to be modified several times from its atomic type, and each modification now requires a copy operation and creation/destruction of another object, is this a significant slowdown?

Not any actually. The shifts and shears don't require a copy, they just attach an SBTransformation to the front of the object. This is already what happens behind the scenes.

obj = galsim.InterpolatedImage(im)
obj.appyShear(shear)
obj.shift(dx,dy)

behind the scenes is really doing (in the C++ layer) something like the following:

sbprofile1 = SBInterpolatedImage(im)
sbprofile2 = SBTransform(sbprofile1, [a,b,c,d matrix from the shear], 0,0)
sbprofile3 = SBTransform(sbprofile2, 0,0,0,0, dx, dy)

Then sbprofile3 is attached back into the original python layer object. My proposal is to stop overwriting the python layer sbprofile attribute with the new values but instead return a new GSObject that wraps the new profile.

[rmandelb]

This is a pretty big API change.

Can we frame this in terms of pros vs. cons? The pro is, as you said, that it's more consistent with how the python = operator works to have these objects be immutable. The only con I can think of is the API change after we already have a bunch of users. Are there any other pros / cons that I'm missing.

I agree that if they are immutable and we don't have to worry about a distinction between creating a new thing vs. changing an old one, it's very natural to turn the methods into rotate, shear, and so on (and I like the idea of createLensed turning into lens). Though, again, it's a big change for users who might already have substantial code written...

[rmjarvis]

@jmeyers314, @dkirkby, @michaelhirsch As three of our users with probably some of the largest code bases, it would be worth getting your input on this proposal. Would you mind switching all of your code that uses setFlux, applyShear, etc. to the above syntax?

For at least the next several releases, the old versions would still work -- they would just raise deprecation warnings. But the plan would be to phase them out eventually.

[dkirkby]

Mike - I am using set/create/apply methods in O(100) places in various bits of code, so I would be affected by any changes but not enough to stand in the way of improving the design.

However, I didn't really follow your point about preferring immutable classes in python. I agree that python's operator=() takes some getting used to (and isn't simply pass by value or pass by reference), but doesn't it work the same way for mutable and immutable objects?

I don't see anything inherently bad with designing mutable classes and would even argue that the language design suggests that user classes should normally be mutable, since attributes are read/write by default (and you have to go to some trouble to prevent that behavior). Put another way, how is GSObject.applyShift any worse than list.append? The thing that bothers me about applyShift, etc, is the fact that you downcast self to GSObject (why is that necessary?) but that's a separate issue to the class' mutability.

[rmjarvis]

The thing that bothers me about applyShift, etc, is the fact that you downcast self to GSObject (why is that necessary?) but that's a separate issue to the class' mutability.

In a way, that is the essence of why I want to make this change. If you start with a Sersic (say) and then do gal.applyShear(shear), then things like gal.getHalfLightRadius() don't make much sense anymore. So to prevent them from being called, we downcast to GSObject.

More to the point, if we didn't downcast, and you tried to do gal.getHalfLightRadius(), it would fail, since the sbprofile attribute (from the C++ layer) is no longer an SBSersic object. It is now an SBTransform object, so it doesn't have that method anymore. The code would raise some kind of exception that would probably not be very easy for the user to parse.

By the way, this behavior is most of the reason that it is hard to make GSObjects picklable. It's hard to keep track of which transformations have been applied to the object, which you would need to do in order to pickle it correctly. c.f. Issue #218.

If instead, each transformation returned a new object, it would be easy to keep track and get the pickling correct. You just take the serialization of the adaptee, add on the transformation, and you're done.

You're right though that this change doesn't really require making the GSObjects immutable. But once we take away the functions that involve SBTransforms wrapping around the original class, then there aren't very many mutating functions left. Basically just scaleFlux and setFlux in most cases. If people prefer, I guess I don't mind leaving them as they are. My gripe is really just with the way we do the transformations.

[dkirkby]

I can think of two common design patterns that address to this problem of applying transforms to a variety of primitive types (Gaussian, Sersic, etc). The obvious approach is that the primitive types all have a common base class X, and then you provide a Transformed class that inherits from X and has the original thing it transforms as an attribute, e.g.

src = galsim.Gaussian(sigma=1)
rotated = src.createRotated(...)
# original object's attributes are still accessible if you know its type
print rotated.original.sigma

The alternative design is to keep a list of applied transforms in the base class X and declare that attributes of the original object refer to the untransformed object, even after transforms have been applied (except perhaps for attributes that are well defined under all possible transforms), e.g.

src = galsim.Gaussian(sigma=1)
src.applyRotation(...)
# original object's attributes are not changed by any transforms
print src.sigma

In either case, downcasting is avoided, the untransformed object's attributes are still accessible and well defined, and serialization should be straightforward. Both approaches allow multiple transforms to be chained or collapsed. Note that these patterns provide either createRotated or applyRotation but not both. Perhaps the underlying problem here is that you are trying to do both?

[rmjarvis]

The C++ layer is doing something similar to your first case. The python layer is currently trying to shoehorn that functionality into a syntax where the object is seemingly modified in place (with the apply functions). What I want to do is to basically make the python layer look more like your first case, which would match up much more closely with what the backend is really doing.

[barnabytprowe]

Hi all,

I've been thinking about this a bit, hence the slight delay posting. First off, I definitely agree with @rmandelb that this is a big API change. It changes pretty much every line of existing, GalSim-calling code that ever applied a transformation to a GSObject.

One proposal I would submit then is that we make whatever we agree on, if it changes a lot of the basic GSObject interface, the starting point of GalSim 2.X. I have the following reasons:

• The change in the primary digit alone would alert users to expect something major to have changed.
• It suits the nature of the changes: I think these are primarily to make the interface cleaner logically, to throw up less coding surprises (e.g. the downcasting seeming a bit odd) and ultimately be more aesthetically pleasing, but they are not urgent.
• GalSim users who don't want to change every line in their code that uses an .applyX or .setY method can just continue to use GalSim 1.X (easier to remember than selecting the correct v1.Y where Y < Z). We can say that after active development to v2 begins the only changes that will get made in GalSim 1.X will be bug fixes (after an even later date we can leave it AS IS with a nice big user warning about how they should upgrade).

Ultimately this is all just presentation, but I would feel a lot more comfortable talking about a big set of changes like this if we had a clear idea for rolling it out in a way that didn't inconvenience all our users (and ourselves included ;) and make the GalSim project as a whole seem unstable.

As for the shape of the future GSObjects, I think I can safely say that I never had (and still don't have) any real issue with the GSObjects being mutable. I think that pure immutability might be difficult to achieve in the more complex RealGalaxy objects, which store things - for example the noise correlation function classes used by the RealGalaxy objects keep and update (on every .draw() call) a cache of (square rooted) discrete noise power spectra so as to potentially speed up future .draw() calls. The same objection goes for the cache in InterpolatedImage objects. There are good practical reasons for wanting these caches, as much as there are good reasons for wanting to make GSObjects picklable. I don't mind ditching setFlux and scaleFlux, but the caches are useful.

All that said, I do also find the way we do transformations a bit unsatisfactory. It seems wrong to silently change Class in a way that will come as a surprise to many. I do like this pattern:

src = galsim.Gaussian(sigma=1)
rotated = src.createRotated(...)
# original object's attributes are still accessible if you know its type
print rotated.original.sigma

Primarily I like that useful information is not destroyed, e.g. if I wanted to, as a user I could work out a value for some definition of the transformed object's sigma, using the .original attribute and a knowledge of coordinate geometry. This is not currently possible and this always seemed a shame.

Using this pattern you also ought to be able to make the vast majority of GSObjects pickable too (I think). Those that cannot be pickled, well, we have a reason. We would need to put some safeguards in place to stop users successively making objects that end up taking vast amounts of memory via endless original.original.original.original.original.original etc. attributes. But that notwithstanding, I like the idea.

P.S. On a related topic, should we think about making the C++-layer SBProfile objects unit flux always? The flux could be a separate attribute then, settable and gettable as a standard float or in, and the ability to change flux at will (which is handy) need not trigger an new object with an .original attribute, which seems unnecessary. Nor need it preclude pickling....

P.P.S. GalSim users do keep coming out of the woodwork so this really is a possibly large impact to a not-insignificant group of people... Some more to add to the discussion list for this are, I think, @emhuff , @joezuntz, @tomaszkacprzak , @suchyta1 ... will try to think of more!

[jmeyers314]

Just looked through my code. I think this would only require O(10) changes for me, so no concern there.

For usability, I rather like the idea of making GSObject and ChromaticObject immutable. As I was driving to work today I was trying to decide what the following code snippet would do:

import galsim
gal=galsim.Gaussian(fwhm=1.0)
pix=galsim.Pixel(0.2)
final=galsim.Convolve(gal, pix)
image1=galsim.ImageF(32, 32, scale=0.2)
image2=galsim.ImageF(32, 32, scale=0.2)
final.draw(image=image1)
gal.setFlux(2.0)
final.draw(image=image2)
print image1.array.max()
print image2.array.max()

Whether or not the two print statements above produce the same output (I'll let you try this out for yourself) is actually besides the point, I think, which is really that I had to run the code to find out. (On the other hand, maybe I'm just really bad at predicting python code, and this is perfectly obvious to everyone else :) ) If GSObjects were immutable, however, then I think I could have confidently predicted the result.

As for the name .setFluxTo(), maybe .withFlux() is another candidate (and grammatically mirrors SED.atRedshift() which @rmjarvis suggested for the SED class).

[rmjarvis]

One proposal I would submit then is that we make whatever we agree on, if it changes a lot of the basic GSObject interface, the starting point of GalSim 2.X.

I think it would be possible to add the new syntax while keeping the old for now, with a comment that they will be deprecated in version 1.Y, and removed in 2.0.

We would need to put some safeguards in place to stop users successively making objects that end up taking vast amounts of memory via endless original.original.original.original.original.original etc. attributes.

I don't think it would be hard to roll all transformations into a single front end. So after final = gauss.shear(shear).rotate(theta).shift(dx,dy), you would be able to access sigma as final.original.sigma.

[rmjarvis]

Whether or not the two print statements above produce the same output (I'll let you try this out for yourself) is actually besides the point, I think, which is really that I had to run the code to find out.

This surprised me. I predicted the opposite, although I do (now) understand why it works that way. It involves something that we thought was just an implementation detail, but it turns out that it gets messed up when the object gets changed after going into a Convolve.

I think this is another argument in favor of moving toward making the GSObjects immutable, although one I hadn't realized before.