Inputs to a multi-object capable GalSim

[barnabytprowe]

If we wish to create multiple object output in GalSim, we need to be able to take input configuration files / catalogues / something to specify either parameters for each object or a rule for selecting parameters.

[barnabytprowe]

Hi Mike...

So, I got my version working too, at least on paper... But I'm finding that the image generation is very slow in comarison (not the I/O handling, the drawing), so I think I must have mucked something up. I'm too tired to debug tonight, and will have to take a look tomorrow.

The development at my end also occured way more slowly than you as predicted! Like you however, I was forced to split the kwargs into three groups: required / req, size / size_opt and optional / opt (my names / your names). These changes in my scheme are instead reflected in the galsim.object_param_dict structure in base.py, which is now a nested dictionary.

If nothing else, this has been a valuable excursion into deeper pythomania for me. Comparing the branches, mine is around 30 lines of code fewer... ;) But of course a) mine is still manifesting something odd and b) it's clarity and maintainability that's important, and so I think tomorrow we should get some second opinions.

Good night!

[rmjarvis]

I made some comments on your code, marked with MJ. In particular, I think the bug you noticed is simply that you need to use col-1 when accessing data. Although, when I had made the same mistake, it led to a crash from SBProfile when the scale size was negative, not just very slow, so maybe there is something else too.

[TallJimbo]

I haven't been following this discussion thoroughly, but I thought I'd mention that having the config schema specified up front allows a very nice solution to this problem: a node in the configuration tree can be a full-fledged specific class (like on of these ObjectBuilders) instead of an AttributeDict, and we can put the method to create the thing we want directly in that class. If we do that fully hierarchically, that means we can basically just eval the configuration data, and then ask it to create all the things it specifies.

I don't think this is the right time to make that switch, but it may be worth keeping that design in mind as what we may want to ultimately use, and not to spend too much time cleaning up corner cases on an AttributeDict-based solution.

[barnabytprowe]

Mike, thanks for the bug fixes! Things are working great. I've added a few changes to MultiObjectDemo.py outside of my own personal frontend.py that you might want to take a look at:

1. I've rebuilt the make_default_input.py and catalog to make the half_flight radii correct. Before they were multiples of the pixel size, whereas really that should be of the PSF FWHM (I might be missing a factor of two still though, but the images look good).
2. In MultiObjectDemo.py (MOD.py), I fixed a couple of things in Script 2.
   • The config.gal.items = [AttributeDict()] * 2, thanks to the Python data model, actually makes both items in the list references to the same object. So if you write into one (as below this line in the script), you write into the other too! This is fixed with the current syntax... I think handling arbitrary length list inits in this way would have to be done with a loop and .append() :S
   • I changed the pixel_scale to 1., since the GREAT08 paper values I stole for sizes are in pixels.
   • I did a very kludgy shearing, just to check it works OK. I think this is either something we need to put into BuildGSObject or handle outside... I think the former might be preferable in general as it would hide all of that dictionary surfing we do from the casual user. Thoughts?
   • I did a kludgy cube output for checking. This will be superseded by your efforts on #102, of course.

Once I added the shears, I did notice that the code slowed down slighty (changing the pixel scale didn't have much effect). I think we're still above a Hz, but not much. On the other hand, deVaucs are just a bitch to sample properly.

Please take a look. If you'd like me to git "cherry pick" any of the non-frontend.py commits into your branch, let me know! (it's v. easy with Sourcetree)

[barnabytprowe]

Jim: I think that's a good point. But agree that we're probably not ready to lock down the interface yet, we just wrote it ;)

[rmjarvis]

Since it seems like more people prefer your style than mine, let me point out a few more changes that I think should be made to your code:

• The if config.type in op_dict line in BuildGSObject should be after all the special cases. That way if we decide to specialize one of them, it can have its own elif line that hits before the generic case that just calls BuildSimple
• I still think Pixel should be so specialized since it does work differently. Then you can put in the check for at least 1 size specification in GetSizeKwargs. If we let that pass to the constructor, the exception that gets raised will look like an error in our code rather than an error in the user's configuration file.
• When you loop over items for Add or Convolve, you need to recurse onto BuildGSObject, not BuildSimple. The way you have it now, you can't Add or Convolve a SquarePixel (for example) with something else.
• The AttributeError in GetRequiredKwargs should inform the user which config.type was in error.
• The InputCatalog part of GetParamValue should be broken out into its own function. We're going to have a lot of these, so if we put them all here, this function will be very long. Better to put each one (or at least the longer ones like InputCatalog) in separate functions.

[rmjarvis]

Now, some thoughts on implementing the shift and shear. I see two possible ways to go here:

1. Make Shifted, Sheared, Rotated all possible types, with associated attributes base and (shift,shear,theta) respectively. So the config file for the implementation that is implied by the current input catalog we're using for script 2 would have to change to:

    config.gal.type = 'Shifted'
    config.gal.base.type = 'Sum'
    config.gal.base.items = [galsim.AttributeDict(), galsim.AttributeDict()] 
    config.gal.base.items[0].type = 'Sheared'
    config.gal.base.items[0].base = 'Exponential'
    config.gal.base.items[0].base.half_light_radius.type = 'InputCatalog'
    config.gal.base.items[0].base.half_light_radius.col = 10
    config.gal.base.items[0].shear.type = 'G1G2'
    config.gal.base.items[0].shear.g1.type = 'InputCatalog'
    config.gal.base.items[0].shear.g1.col = 11
    config.gal.base.items[0].shear.g2.type = 'InputCatalog'
    config.gal.base.items[0].shear.g2.col = 12
    config.gal.base.items[0].flux = 0.6 * gal_flux
    config.gal.base.items[1].type = 'Sheared'
    config.gal.base.items[1].base = 'DeVaucouleurs'
    config.gal.base.items[1].base.half_light_radius.type = 'InputCatalog'
    config.gal.base.items[1].base.half_light_radius.col = 13
    config.gal.base.items[1].shear.type = 'G1G2'
    config.gal.base.items[1].shear.g1.type = 'InputCatalog'
    config.gal.base.items[1].shear.g1.col = 14
    config.gal.base.items[1].shear.g2.type = 'InputCatalog'
    config.gal.base.items[1].shear.g2.col = 15
    config.gal.base.items[1].flux = 0.4 * gal_flux
    config.gal.shift.type = 'DXDY'
    config.gal.shift.dx.type = 'InputCatalog'
    config.gal.shift.dx.col = 16
    config.gal.shift.dy.type = 'InputCatalog'
    config.gal.shift.dy.col = 17

This would be most consistent with the structure that we've been developing so far. But the proliferation of layers of attributes is starting to be a bit much.

1. The other way that I've thought of is to allow every GSObject that gets built to have optional attributes of shift, shear, and rotation. This is much closer to the config file that I already wrote. Just need to add in the G1G2 specification, which I think we want, since we also want other varieties like E1E2, GTheta, ETheta, and possible 2-d random generator types. So the config file would become:

    config.gal.type = 'Sum'
    config.gal.items = [galsim.AttributeDict(), galsim.AttributeDict()] 
    config.gal.items[0].type = 'Exponential'
    config.gal.items[0].half_light_radius.type = 'InputCatalog'
    config.gal.items[0].half_light_radius.col = 10
    config.gal.items[0].shear.type = 'G1G2'
    config.gal.items[0].shear.g1.type = 'InputCatalog'
    config.gal.items[0].shear.g1.col = 11
    config.gal.items[0].shear.g2.type = 'InputCatalog'
    config.gal.items[0].shear.g2.col = 12
    config.gal.items[0].flux = 0.6 * gal_flux
    config.gal.items[1].type = 'DeVaucouleurs'
    config.gal.items[1].half_light_radius.type = 'InputCatalog'
    config.gal.items[1].half_light_radius.col = 13
    config.gal.items[1].shear.g1.type = 'InputCatalog'
    config.gal.items[1].shear.g1.col = 14
    config.gal.items[1].shear.g2.type = 'InputCatalog'
    config.gal.items[1].shear.g2.col = 15
    config.gal.items[1].flux = 0.4 * gal_flux
    config.gal.shift.type = 'DXDY'
    config.gal.shift.dx.type = 'InputCatalog'
    config.gal.shift.dx.col = 16
    config.gal.shift.dy.type = 'InputCatalog'
    config.gal.shift.dy.col = 17

The main advantage of this option is that it keeps the config file a bit flatter (no "base" level). The main disadvantage I see is the order of operations ambiguity of the shift, shear and rotation. They don't commute. So we need to either declare which order they are applied or allow the user to specify. Probably shift should always be last. But I can easily think of examples where we would want either shear or rotation to precede the other.

[rmjarvis]

Another improvement would be to allow all GSObjects to take a flux attribute. This isn't currently allowed for Sum, since Add doesn't take a flux as part of its constructor. However, it does have a setFlux, as all GSObjects do. So I think the check for a flux attribute should be made at the end of BuildGSObject and use the setFlux method to set what flux to use. Then the flux lines in the config file could be changed to:

config.gal.items[0].flux = 0.6
config.gal.items[1].flux = 0.4
config.gal.flux = gal_flux

which I think is clearer, and less prone to error. e.g. for even more complicated combinations, it might be hard to propagate all the flux values correctly down into the relevant components.

[rmjarvis]

Hmm. Actually, my second method for sheart/rotation doesn't actually work for the Great08 image script. In that one, the galaxies are first sheared in the g1 direction. Then rotated a random amount. Then sheared by the gravitational signal.

Well, maybe that would still be ok. We probably want to have the applied gravitational shear be its own top level anyway: config.shear, rather than hide it in the galaxy level. So the g1,g2,etc. in the gal level of the hierarchy would just define the intrinsic shape of the galaxy. That's probably cleaner.

[rmandelb]

Mike, sorry if I'm missing something here, but if >1 of shear/rotation is specified, then we also need a way of specifying an order of operations. Is that assumed to come from the order in which the operations are listed?

And just to make sure I understand, if we have a 2-component galaxy as in your examples, then the config.gal.base.items[i].shear specify the galaxy intrinsic shape, and then if we want to impose a lensing shear on the composite galaxy, we'd do that with config.gal.shear?

On May 11, 2012, at 6:11 AM, Mike Jarvis wrote:

Now, some thoughts on implementing the shift and shear. I see two possible ways to go here:

1. Make Shifted, Sheared, Rotated all possible types, with associated attributes base and (shift,shear,theta) respectively. So the config file for the implementation that is implied by the current input catalog we're using for script 2 would have to change to:

   config.gal.type = 'Shifted'
   config.gal.base.type = 'Sum'
   config.gal.base.items = [galsim.AttributeDict(), galsim.AttributeDict()] 
   config.gal.base.items[0].type = 'Sheared'
   config.gal.base.items[0].base = 'Exponential'
   config.gal.base.items[0].base.half_light_radius.type = 'InputCatalog'
   config.gal.base.items[0].base.half_light_radius.col = 10
   config.gal.base.items[0].shear.type = 'G1G2'
   config.gal.base.items[0].shear.g1.type = 'InputCatalog'
   config.gal.base.items[0].shear.g1.col = 11
   config.gal.base.items[0].shear.g2.type = 'InputCatalog'
   config.gal.base.items[0].shear.g2.col = 12
   config.gal.base.items[0].flux = 0.6 * gal_flux
   config.gal.base.items[1].type = 'Sheared'
   config.gal.base.items[1].base = 'DeVaucouleurs'
   config.gal.base.items[1].base.half_light_radius.type = 'InputCatalog'
   config.gal.base.items[1].base.half_light_radius.col = 13
   config.gal.base.items[1].shear.type = 'G1G2'
   config.gal.base.items[1].shear.g1.type = 'InputCatalog'
   config.gal.base.items[1].shear.g1.col = 14
   config.gal.base.items[1].shear.g2.type = 'InputCatalog'
   config.gal.base.items[1].shear.g2.col = 15
   config.gal.base.items[1].flux = 0.4 * gal_flux
   config.gal.shift.type = 'DXDY'
   config.gal.shift.dx.type = 'InputCatalog'
   config.gal.shift.dx.col = 16
   config.gal.shift.dy.type = 'InputCatalog'
   config.gal.shift.dy.col = 17

This would be most consistent with the structure that we've been developing so far. But the proliferation of layers of attributes is starting to be a bit much.

1. The other way that I've thought of is to allow every GSObject that gets built to have optional attributes of shift, shear, and rotation. This is much closer to the config file that I already wrote. Just need to add in the G1G2 specification, which I think we want, since we also want other varieties like E1E2, GTheta, ETheta, and possible 2-d random generator types. So the config file would become:

   config.gal.type = 'Sum'
   config.gal.items = [galsim.AttributeDict(), galsim.AttributeDict()] # The Python data model
                                                                       # means that [ad()]*2 fails!
   config.gal.items[0].type = 'Exponential'
   config.gal.items[0].half_light_radius.type = 'InputCatalog'
   config.gal.items[0].half_light_radius.col = 10
   config.gal.items[0].shear.type = 'G1G2'
   config.gal.items[0].shear.g1.type = 'InputCatalog'
   config.gal.items[0].shear.g1.col = 11
   config.gal.items[0].shear.g2.type = 'InputCatalog'
   config.gal.items[0].shear.g2.col = 12
   config.gal.items[0].flux = 0.6 * gal_flux
   config.gal.items[1].type = 'DeVaucouleurs'
   config.gal.items[1].half_light_radius.type = 'InputCatalog'
   config.gal.items[1].half_light_radius.col = 13
   config.gal.items[1].shear.g1.type = 'InputCatalog'
   config.gal.items[1].shear.g1.col = 14
   config.gal.items[1].shear.g2.type = 'InputCatalog'
   config.gal.items[1].shear.g2.col = 15
   config.gal.items[1].flux = 0.4 * gal_flux
   config.gal.shift.type = 'DXDY'
   config.gal.shift.dx.type = 'InputCatalog'
   config.gal.shift.dx.col = 16
   config.gal.shift.dy.type = 'InputCatalog'
   config.gal.shift.dy.col = 17

The main advantage of this option is that it keeps the config file a bit flatter (no "base" level). The main disadvantage I see is the order of operations ambiguity of the shift, shear and rotation. They don't commute. So we need to either declare which order they are applied or allow the user to specify. Probably shift should always be last. But I can easily think of examples where we would want either shear or rotation to precede the other.

---

Reply to this email directly or view it on GitHub: https://github.com/GalSim-developers/GalSim/issues/101#issuecomment-5647653

---

Rachel Mandelbaum http://www.astro.princeton.edu/~rmandelb rmandelb@astro.princeton.edu

[rmjarvis]

In option 1 above (with the base's), it's all very explicit. So if you want to shear, then rotate, then shear again, then shift, you have four hierarchy levels that specify exactly the order you want. Perhaps a concrete example would help, so here is how the specifications of the MultiObjectDemo script 1 (in either this branch or #103) would look with this model (making up some stuff for some of the value specifications):

gal.type = 'Shifted'
gal.shift.type = 'TopHat'
gal.shift.radius = gal_centroid_shift'
gal.base.type = 'Sheared'
gal.base.shear.type = 'G1G2'
gal.base.shear.g1 = gal_g1
gal.base.shear.g2 = gal_g2
gal.base.base.type = 'Rotated'
gal.base.base.theta.type = 'Ring'
gal.base.base.theta.num = 2
gal.base.base.theta.first.type = 'UniformDeviate'
gal.base.base.theta.first.min = 0
gal.base.base.theta.first.max = 2*math.pi
gal.base.base.base.type = 'Sheared'
gal.base.base.base.shear.type = 'G1G2'
gal.base.base.base.shear.g1.type = 'TruncatedGaussianDeviate'
gal.base.base.base.shear.g1.sigma = gal_ellip_rms
gal.base.base.base.shear.g1.max = gal_ellip_max
gal.base.base.base.shear.g2 = 0
gal.base.base.base.base.type = 'Sersic'

So you can see why I'm leaning pretty heavily away from this model. All those bases are pretty ugly. So on my second one, I think the model would be that the order is shear then rotate then shift. And if there is a top level shear specified, then this is applied after the galaxy's shear and rotation, but before its shift. So the same thing with the second model is:

gal.type = 'Sersic'
gal.shear.type = 'G1G2'
gal.shear.g1.type = 'TruncatedGaussianDeviate'
gal.shear.g1.sigma = gal_ellip_rms
gal.shear.g1.max = gal_ellip_max
gal.shear.g2 = 0
gal.rotation.type = 'Ring'
gal.rotation.num = 2
gal.rotation.first.type = 'UniformDeviate'
gal.rotation.first.min = 0
gal.rotation.first.max = 2*math.pi
gal.shift.type = 'TopHat'
gal.shift.radius = gal_centroid_shift'
shear.type = 'G1G2'
shear.g1 = gal_g1
shear.g2 = gal_g2

This is certainly a lot neater. But I don't know if the order of operations is too confusing this way.

[rmandelb]

The first option looks pretty bad to me with all the base.base.base etc. But I do want to be able to specify order of operations... so, what about something that looks roughly like this:

...a bunch of lines specifying the galaxy model...
gal.operation.number = 4
gal.operation[0] = 'shear'
...a bunch of lines here specifying the shear used to make intrinsically non-round galaxies...
gal.operation[1] = 'rotation'
...a bunch of lines here specifying the rotation...
gal.operation[2] = 'shear'
...a bunch of lines here specifying the top-level lensing shear...
gal.operation[3] = 'shift'
...a bunch of lines here specifying the shift...

or, in the case of multi-component galaxies, we'd have some of the above operations applied to components in a specified order, and some like the shear and shift at the end applied to the summed components, again with a specified order.

[rmandelb]

To complete my thought: this means that we would have freedom to do things that go against the standard order assumption but that people do sometimes find useful, e.g., offsets between galaxy components (which require shifts of components earlier in the process, and which can be used to roughly construct an irregular galaxy model).

[rmjarvis]

Offsets between components already work with my scheme. Each gal.item[k] can have a shift attribute which would be relative to the nominal position of the base galaxy. This shift would be applied after any gal.item[k].shear and gal.item[k].rotation, which I think is what you will always want. Then any shear, rotation or shift at the gal level would be done afterwards. I find it hard to imagine cases where the shift is appropriately done before the rotation or shear for the component in question.

It's still an open question I guess if there are any cases where a shear would be done after a rotation aside from the final applied gravitational shear, which I think is ok to make a special case for.

Furthermore, if someone does want to do something unconventional, we can allow it via type = 'Base', which would do nothing more than add a single layer. So the order of operations would be:

• gal.base.shear
• gal.base.rotation
• gal.base.shift
• gal.shear
• gal.rotation
• gal.shift

But I suspect the use of that would be rare.

Finally, I should point out that your example with operation[k] won't work as written. If operation[k] is a string, then it can't have any further attributes. (e.g. operation[0].g1 = ...) So you at least need something like `gal.operation[0].op = 'shear'.

[TallJimbo]

It really seems to me that we're trying to make the full functionality of the Python module available through the configuration interface, and that seems like a very bad idea.

If someone wants to apply a whole host of transforms to objects in some arbitrary order, they should write custom code to do that.

The configuration/catalog interface should just provide a way to do common things: take a catalog of galaxies, apply some shear to it, convolve it with a PSF, and make images.

[rmjarvis]

Another option just occurred to me. We could have two kinds of "shear" attributes: ellip and shear. So the order would be ellip then rotation then shear then shift. That's probably even clearer, since the point of the first one is to define the true ellipticity of the galaxy. Then the second would be an applied shear after that (typically by gravity).

[rmandelb]

Yes, I agree that shifts of components should still happen after their shears and rotations. I hadn't seen that shifts of components was easily included in your scheme already, sorry. So I guess that does cover most of the typical operations.

Re: the operation[k] stuff, I agree -- it wasn't meant to be an example of working code, just a conceptual illustration.

[TallJimbo]

Also, to step back a bit, why are we putting information about the galaxy types in the configuration file, rather than as records in the input catalog? Do these just provide a template that states how to interpret the records of a catalog?

[rmandelb]

I agree that it would be useful to make two kinds of shear attributes, one for intrinsic ellipticity and one for lensing shear.

[rmjarvis]

It really seems to me that we're trying to make the full functionality of the Python module available through the configuration interface, and that seems like a very bad idea.

If someone wants to apply a whole host of transforms to objects in some arbitrary order, they should write custom code to do that.

That's a good point. We don't need to provide every possible scenario here. But I do think we should be able to easily specify in a config file what is currently done in script1. That's a pretty basic functionality, so it should be fairly easy to specify in our configuration file.

[rmjarvis]

Also, to step back a bit, why are we putting information about the galaxy types in the configuration file, rather than as records in the input catalog? Do these just provide a template that states how to interpret the records of a catalog?

Because reading from a catalog should be just one way to get the information. We don't want to require the user to make a catalog just to do something simple.

[TallJimbo]

Also, to step back a bit, why are we putting information about the galaxy types in the configuration file, rather than as records in the input catalog? Do these just provide a template that states how to interpret the records of a catalog?

Because reading from a catalog should be just one way to get the information. We don't want to require the user to make a catalog just to do something simple.

Well, I think a task should be both simple and common in order to merit special support. But maybe this one is...I don't want to hold up progress on this because I haven't been following it as closely as I'd need to make intelligent comments.

But I think we should try to avoid writing our own little config sub-language instead of expecting users to write scripts themselves when we aren't doing batch processing.

[barnabytprowe]

Hi all, am joining the discussion late, so will have to respond to a couple previous points before wading in in an up to date way...

Since it seems like more people prefer your style than mine, let me point out a few more changes that I think should be made to your code...

OK, I'll do these now, thanks they're helpful points. I don't like making Pixel its own special type, while it does currently work differently I'd prefer to make it work the same and then consistently treat it like all the other 2D profiles we're sampling from, but this is something we can easily discuss later.

Regarding thoughts on the shift and shear...

My initial reaction was strongly in favour of option 2., which I see has been the primary focus of subsequent discussion.

As for the question of how to allow sometimes rotation to happen before shear, and vice versa, between the operations (Rachel's scheme) versus (Mike's) ellip + rotation + shear scheme I think I prefer the latter. It's hardcoding the behaviour we want, and makes physical sense.

In response to Jim:

The configuration/catalog interface should just provide a way to do common things: take a catalog of galaxies, apply some shear to it, convolve it with a PSF, and make images.

I think the default settings should be clear, and we should have good examples for this usage, but I don't really see how the issues being discussed by Rachel and Mike above are non-essential for what you're suggesting. These operations need to happen, and how they happen needs to be defined.

At the very least, I think users should have the option to make certain galaxy parameters constant, or come from a fields in a Catalog - this is what we're currently implementing, and I don't see that as overly-excessive complexity.

why are we putting information about the galaxy types in the configuration file, rather than as records in the input catalog? Do these just provide a template that states how to interpret the records of a catalog?

Yes, currently. They also provide a means of setting constant values. Actually, the specification of galaxy type via catalog entry is (I need to think about this) not something we've currently got working. I think it's possible, but we've not implemented it - this is in fact a level of abstraction/complexity beyond that at which we're currently working!

[rmjarvis]

Maybe it will help if I share an improvement that I've had in mind from the beginning, but that isn't implemented in the code yet.

I'd like the normal case be to not specify the type and related attributes explicitly. Instead, if something is a string, then it would be parsed into the appropriate attributes that would be passed onto our current functions. So my above example for script 1 would be simply:

gal.type = 'Sersic n=1.5 half_light_radius=4'
gal.ellip = 'G1G2 g1=(TruncatedGaussianDeviate sigma=gal_ellip_rms max=gal_ellip_max) g2=0'
gal.rotation = 'Ring num=2 first=(UniformDeviate min=0 max=2*math.pi)'
gal.shift = 'TopHat radius=gal_centroid_shift'
gal.shear = 'G1G2 g1=gal_g1 g2=gal_g2'

So that suddenly becomes a really nice readable configuration file that would be far easier for most users than writing a full-on python script. (Which is not as trivial for some as it is for you.)

[TallJimbo]

To me, that's actually less readable - it's a totally new syntax that someone will have to learn about from documentation regardless of whether or not they know any Python.

But I don't want to be too inflammatory here and Barney's comments about how this relates to catalogs assures me somewhat that this is useful stuff to be working on. I guess I was supposing that the config file needed to populate a field of randomly-generated galaxies would create a catalog, and then we'd go from catalog to GSObjects (which means we'd never actually go from config to GSObjects). But that's not the only way to do it, of course. Anyhow, maybe it's better for you to all to keep working on this until you have something more complete, and for me to wait until I have a chance to look at it more deeply and come up with alternatives before I comment further.

[rmjarvis]

Well, an end user is going to have to read some documentation to use the software.

My hope is that most of them will never need to look at our python documentation at all, and just look at how to write a config file. Then they write the config file they want, and call our driver program with that. Then out pop images.

I actually agree with you that users who are comfortable with python would probably opt to not ever bother with a configuration file at all. They'd just write the python code. But I guarantee that using a configuration file will be easier for a lot of users than writing python code.

[barnabytprowe]

OK, I've merged the relevant changes on master and #103 into this branch, and had a go at Mike's suggested changes (all done, no objections - one winky comment next to the "Pixel" type). Already, this is looking more like a hybrid of the approaches with the main difference now being that Generate = _GetParamValue and the fact that the required, size and optional lists come from a central dictionary.

I propose to set up a pull resuest to merge these changes into master now, and close #143. This branch will stay open for implementing the shear and shifts described above. Will do that now, can close the pull request if strong objections.

[barnabytprowe]

P.S. python MultiObjectDemo.py 2 is also working as required.

[rmjarvis]

Do you want to merge them into master? Or just 103? We can do the latter without a pull request. And I think we could try to implement shift and shear today and get that included in the milestone...

[barnabytprowe]

Was thinking we can do shift and shear still on this branch, but thought it would be good to get these (substantial) changes onto master and continue working from #101. Can pull request more than once from the same branch... Also, this Issue could be used for discussing the shift, shear implementation. the pull request could focus on patching/minor mods/bug fixes to what I currently have - seemed to make sense to separate the two for readability.

On 11 May 2012, at 10:27, Mike Jarvis wrote:

Do you want to merge them into master? Or just 103? We can do the latter without a pull request. And I think we could try to implement shift and shear today and get that included in the milestone...

---

Reply to this email directly or view it on GitHub: https://github.com/GalSim-developers/GalSim/issues/101#issuecomment-5656336

Barnaby Rowe

Jet Propulsion Laboratory California Institute of Technology MS 169-237 4800 Oak Grove Drive Pasadena CA 91109 United States of America

Department of Physics & Astronomy University College London Gower Street London WC1E 6BT United Kingdom

[rmjarvis]

I thought any further changes to branch #101 would automatically get included in the pull request. So doesn't that make it hard to keep working on stuff that we don't want merged with the pull request?

[barnabytprowe]

Argh, you are correct. Rats. OK, spoke too soon!

On 11 May 2012, at 10:37, Mike Jarvis wrote:

I thought any further changes to branch #101 would automatically get included in the pull request. So doesn't that make it hard to keep working on stuff that we don't want merged with the pull request?

---

Reply to this email directly or view it on GitHub: https://github.com/GalSim-developers/GalSim/issues/101#issuecomment-5656543

Barnaby Rowe

Jet Propulsion Laboratory California Institute of Technology MS 169-237 4800 Oak Grove Drive Pasadena CA 91109 United States of America

Department of Physics & Astronomy University College London Gower Street London WC1E 6BT United Kingdom

[barnabytprowe]

OK, closed that. Apologies to all for the spam. Mike, do the current changes in frontend.py work OK for you?

Once the dust settles on that, I suggest the following plan for getting the ellips, rotates, shifts and shears working: you put the config syntax you like best into the MultiObjectDemo.pyscript, and then I'll make sure that BuildGSObject handles it correctly. Then we iterate: your comments very useful always. Sound OK?

[rmjarvis]

Done.

[barnabytprowe]

Thanks Mike, will ping when it's ready for discussion.

[rmjarvis]

While your in there working on it, maybe you can think about an idea I had when writing my version, but couldn't quickly figure out how to do.

When we raise an error about the configuration being wrong, the user is usually only told the 'type' or param_name that had the problem. It would be nice if the error message also could tell them the full name that was originally listed in the parameter file. ie. that an error is for gal.shear.g1 rather than any other g1's that might be around.

I'm not sure if this is possible. It certainly wouldn't be possible in C++, but python seems like it might have a nice way to access this information and make a slightly more helpful error message.

[barnabytprowe]

Hi Mike...

I've pushed one implementation. I've not done error handling for the most part for the prototype, as I figured that sort of thing can be added later and actually obscures readability while prototyping. The are some new functions in frontend.py, hopefully it's all fairly obvious.

To handle both the G1G2 and E1E2 type ellipticities (the latter not currently directly supported by base.py) I've added two new methods to GSObjects. I wasn't sure whether you would prefer to see whole new methods for the ellip transformations, or rather see a more general method with convention switching via a keyword. I've kind of implemented both as an illustration, so we can decide. (In frontend.py I actually call the different methods rather than use the convention switch.)

Comments welcome.

There is one thing I noticed: the galaxy ellipticities seem a little rounder than in my previous kludges to check ellipticities were working in MultiObjectDemo.py. I think this is attributable to the use of the 'E1E2' convention rather than the previous 'G1G2' - it's about a factor of two. I do want to just check this, however, even if only qualitatively.

Note: I've not done the flux yet, and so reinstated the previous scaling - commenting out the previous gal.flux setting too, and raising a NotImplementedError in BuildGSObject if flux is set for Sums or Convovles, just to remind us. I think this merits discussion because...

1. I'm not 100% convinced of the benefits of requiring an extra parameter when one fewer would do.
2. I would prefer to see the individual component fluxes labelled flux_frac or something similar in the scheme you outline above, and yet that would seem to necessitate a renaming of the flux parameter (in either mine or your frontend.py schemes) or a reworking of the system.

Given my reservations about 1., I'd rather delay 2. until there's more time to discuss it.

[barnabytprowe]

Mike: in response to your point for pondering... One thought is that it could be done by only ever passing the lowest-level config along with a (possibly empty or variable length) list of strings to use to access the nested attributes. It's a little clunkier I think, but it could be used to print out exactly the sorts of error messages you describe, which would definitely be really nice.

[barnabytprowe]

P.S. When I say I'll add the exception handling later in the first of the two message above, I mean later today!

[rmjarvis]

1. I see you don't implement ellip, shift, etc. for Pixel or SquarePixel. I think that's right. At least I can't think of any reason why someone would want to do any of these modifications for a pixel. But I figured I'd comment on it here in case it raises any red flags for the others reading these comments.
2. Rotation not supported yet. Check. Need to think about the right syntax for that. We could have the type be Degrees, Radians, etc. (Typically just those two.) And then both require theta = the angle in those units.
3. I don't think we need separate BuildEllipObject and BuildShearObject, since they are identical (modulo the slight difference in error message) if you pass them config.ellip and config.shear rather than just pass config. (In parallel with how you do _GetParamVale)
4. The order is slightly wrong. shear is before shift.
5. The "E1E2" meaning is called Distortion in base.py. So, gsobject.applyDistortion(e1,e2).
6. For all of them the code could be abbreviated somewhat by using the apply methods rather than create methods. e.g. gsobject = gsobject.createSheared(g1, g2) is equivalent to gsobject.applyShear(g1,g2).

[barnabytprowe]

P.P.S. After qualitative checks on the images from making all the ellipticities g-type, i.e. |g| = (a - b) / (a + b), things look as they did before. Except that now we also have nice centroid offsets. Seems to be working OK.

[barnabytprowe]

I don't think we need separate BuildEllipObject and BuildShearObject, since they are identical (modulo the slight difference in error message) if you pass them config.ellip and config.shear rather than just pass config. (In parallel with how you do _GetParamVale)

OK, would you rather it was called BuildEllipObject or Shear? I think the former is probably better in general (only galaxies are sheared whereas both galaxies and PSFs have ellipticities)

The order is slightly wrong. shear is before shift.

Ooops, of course. This is why I need your help Mike! :)

The "E1E2" meaning is called Distortion in base.py. So, gsobject.applyDistortion(e1,e2).

Almost, now I look properly it's gsobject.applyDistortion(galsim.Ellipse(e1, e2)) but point well made, I'll change that.

For all of them the code could be abbreviated somewhat by using the apply methods rather than create methods. e.g. gsobject = gsobject.createSheared(g1, g2) is equivalent to gsobject.applyShear(g1,g2).

Good point, I clearly still think in a function() -> return value kind of way straight out of the gate. Will change.

I'll add some clear comments about the fact we don't shear / shift pixels into the code, then push the above and with some exception handling.

[rmjarvis]

I think we do want to have a flux setting for the overall galaxy. e.g. This is the thing that would probably be listed in the catalog if there were a flux in there. Not each component flux separately. Or you might want to have the flux come from some kind of distribution. It would be difficult to do that correctly if you are forced to specify the component fluxes separately.

Plus if someone wants to do it your way, that's also supported. No need to specify an overall flux. If not specified, the SBAdd object determines the flux from the sum of the components, so that's fine.

And I think it's clear enough to have the flux of the components mean the fractional flux if an overall flux is specified. (We would have to make clear what the convention is if the component fluxes don't add to 1, but that's the only way there would be an ambiguity I think.)

On the other hand, I'm fine with introducing a new option of flux_frac if you think that would be clearer. I guess it would only be available on components and it would mean the same thing as flux, except that we might force the sum to be 1 (either by raising an exception if it doesn't or rescaling).

[barnabytprowe]

:) OK, I'll put it in, but I would like to discuss it a little more post milestone!

[barnabytprowe]

Changes pushed to match your comments (sorry, pushed onto #103 rather than #101 but I'm keeping them merged and in sync at the moment).

The flux is added as in the original scheme too - I've not changed to flux_frac, but will think about this question a little more. All is working on the demo 2 script as before.

Now I'll add the exceptions.

[rmandelb]

Hi guys -

I'm not following much right now, but I was scanning and noticed this:

1. I see you don't implement ellip, shift, etc. for Pixel or SquarePixel. I think that's right. At least I can't think of any reason why someone would want to do any of these modifications for a pixel. But I figured I'd comment on it here in case it raises any red flags for the others reading these comments.

In Demo.py we sheared a Pixel as an approximation for distortion.

If we want to use Pixel objects to populate a mask image representing artifacts, we might want to be able to shift them to cover different locations in an image? (Not sure that that should be done using shift, probably there's a better way.)

[rmjarvis]

What do you think about my comment that it would be nice to have Config as an alias for AttributeDict? So just add

Config = AttributeDict

in base.py. And then an empty config object could be made with:

config = Config()

Rationale: The name AttributeDict is basically named after its implementation. It stores attributes in a __dict__. It's pretty snazzy really. But the user doesn't care. She just wants something that implements the features of a Configuration file, so Config() seems like a better term to use for that.

[barnabytprowe]

Alright Mike, I think the only exceptions that need to be raised are for whether "type" in config.__dict__... Can you see more? Otherwise, I think this is working OK.

I have to confess, I don't fully remember the plan. Was it to merge this stuff into the master and call it quits for this week after that (addressing script 3 with real galaxies next week)? In that case, we might consider a few last checks before a pull request, or did you have more in mind for #103?

[rmjarvis]

In Demo.py we sheared a Pixel as an approximation for distortion.

I thought about this. But I think we should handle that as part of some (future) distortion configuration. Because handling it right is pretty non-trivial. I sheared the pixels with the negative of what I eventually sheared the final profile by so that the effective pixel being drawn ended up square. I'm still not positive that that's right.

In any case, when we eventually want to implement that in an automated way, we should test what the right thing to do is and not require the user to go through those machinations by hand.

[rmandelb]

Fair enough.

On May 11, 2012, at 5:18 PM, Mike Jarvis wrote:

In Demo.py we sheared a Pixel as an approximation for distortion.

I thought about this. But I think we should handle that as part of some (future) distortion configuration. Because handling it right is pretty non-trivial. I sheared the pixels with the negative of what I eventually sheared the final profile by so that the effective pixel being drawn ended up square. I'm still not positive that that's right.

In any case, when we eventually want to implement that in an automated way, we should test what the right thing to do is and not require the user to go through those machinations by hand.

---

Reply to this email directly or view it on GitHub: https://github.com/GalSim-developers/GalSim/issues/101#issuecomment-5661465

---

Rachel Mandelbaum http://www.astro.princeton.edu/~rmandelb rmandelb@astro.princeton.edu