barnabytprowe / great3-public

Public repository for the Third Gravitational Lensing Accuracy Testing Challenge
BSD 3-Clause "New" or "Revised" License
9 stars 11 forks source link

fix postage stamp bounds in extractPSF #7

Closed dmargala closed 10 years ago

dmargala commented 10 years ago

The great3 image bounds are zero-indexed. It looks like the extractPSF function assumes they are one-indexed.

Minor bug, probably doesn't change things too much...

For example:

Setup:

import galsim
starfield_im = galsim.fits.read("/Users/daniel/data/great3/control/ground/constant/starfield_image-000-0.fits")
size = 48

This is okay:

starfield_im[galsim.BoundsI(0,size-1,0,size-1)]
<galsim._galsim.ImageViewF at 0x107f591c0>

This is not:

starfield_im[galsim.BoundsI(-1,size-2,-1,size-2)]
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-12-e6dd3c2bc4d8> in <module>()
----> 1 starfield_im[galsim.BoundsI(-1,46,-1,46)]

/usr/local/lib/python2.7/site-packages/galsim/image.py in Image_getitem(self, key)
    161 
    162 def Image_getitem(self, key):
--> 163     return self.subImage(key)
    164 
    165 # Define a utility function to be used by the arithmetic functions below

RuntimeError: Image Error: Subimage bounds (-1 46 -1 46 ) are outside original image bounds (0 143 0 143 )
msimet commented 10 years ago

Whoops, thanks for catching this! The fix looks good to me...I'll let another person look at it (we usually do >=2 person code reviews on PRs) and if everything's OK we can merge it.

(One thing that @rmjarvis pointed out elsewhere--this can also be fixed via a call to galsim.setOrigin. Given the way the images are passed around here, I think your fix is the less confusing option, but I'll let others chime in.)

rmandelb commented 10 years ago

Mea culpa - that is my bug, so thank you for catching it!

Sorry for the very long delay in commenting on this. I was away most of last week and have still not caught up on everything that happened while I was gone.

To address Melanie's comment: I think that if we weren't taking a sub image, the simplest fix is to use the setOrigin method, but since we're taking a sub-image it makes sense to do it the way Daniel has written it.

I will merge this tonight unless anyone objects in the next few hours.

rmandelb commented 10 years ago

Merging! Thanks again, @dmargala !

mtewes commented 10 years ago

Thanks indeed ! This made me discover that the indexing convention is influenced by some FITS keywords. So when reading in a GREAT3 FITS image with galsim.fits.read(), the BoundsI convention is (0, size-1, 0, size-1). But when reading any normal FITS image, or when creating an image using e.g. img = galsim.ImageF(100,100), bounds start at 1 (by default). Cheers, Malte

rmandelb commented 10 years ago

Right. I just dug into this to remind myself why the GREAT3 FITS images have this bound convention, and I'm pretty sure it's because we wrote the configuration files with a keyword index_convention : python. ( @rmjarvis , can you confirm whether this is what made config set the index convention to start at 0?) I think we in general do want GalSim's default to be consistent with the FITS convention of starting at 1, and that seems to be the case; I can't remember why we decided to override this for GREAT3, but it's not a significant enough problem to warrant regenerating anything.

rmjarvis commented 10 years ago

it's because we wrote the configuration files with a keyword index_convention : python

Yes, that is the proximate reason. But we chose that because of some convenience with the calculations of the positions. I don't remember specifically now what that convenience was though.