GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
227 stars 107 forks source link

rename `withOrigin` to `shiftOrigin` for WCS classes? #1073

Closed beckermr closed 4 years ago

beckermr commented 4 years ago

I am working with a TanWCS and am seeing the following.

This bit of code illustrates what is going on

In [3]:     origin1 = galsim.PositionD(x=10, y=20) 
   ...:     origin2 = galsim.PositionD(x=20, y=40) 
   ...:     shift = origin2 - origin1 
   ...:     scale = 0.25 
   ...:     world_origin = galsim.CelestialCoord( 
   ...:         ra=10 * galsim.degrees, dec=12 * galsim.degrees) 
   ...:     wcs = galsim.TanWCS( 
   ...:         affine=galsim.AffineTransform( 
   ...:             scale, 0, 0, scale, 
   ...:             origin=origin1, 
   ...:             world_origin=galsim.PositionD(0, 0), 
   ...:         ), 
   ...:         world_origin=world_origin, 
   ...:         units=galsim.arcsec, 
   ...:     ) 
   ...:     print("origin1|wcs.toImage(world_origin):", origin1, wcs.toImage(world_origin)) 
   ...:     print("origin2|wcs.withOrigin(origin2).toImage(world_origin):", origin2, wcs.withOrigin(origin2).toImage(world_origin)) 
   ...:     print("shift|wcs.withOrigin(origin2).toImage(world_origin):", shift, wcs.withOrigin(shift).toImage(world_origin)) 
   ...:                                                                                                                                                                                                 
origin1|wcs.toImage(world_origin): galsim.PositionD(10.0,20.0) galsim.PositionD(10.0,20.0000000000229)
origin2|wcs.withOrigin(origin2).toImage(world_origin): galsim.PositionD(20.0,40.0) galsim.PositionD(30.0,60.0000000000229)
shift|wcs.withOrigin(origin2).toImage(world_origin): galsim.PositionD(10.0,20.0) galsim.PositionD(20.0,40.0000000000229)

Not that if I call withOrigin with where I want world_origin to point, It adds to the old origin. If instead I feed it a shift, it does what (at least) I expect.

cc @rmjarvis

beckermr commented 4 years ago

Further @rmjarvis pointed out that in the code for images, this is used by feeding it a delta/shift

https://github.com/GalSim-developers/GalSim/blob/releases/2.2/galsim/image.py#L1103

rmjarvis commented 4 years ago

Probably the upshot is that we should rename the WCS withOrigin function to be shiftOrigin, which would match what it actually does.
I think the documentation here is probably accurate. And the name is ok for LocalWCS's, since it gives them an origin when they didn't really have one. But for things that already have a non-zero origin, the name is confusing and shiftOrigin is probably more appropriate.

rmjarvis commented 4 years ago

Done. #1085