search

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

Star catalog centroids #1

Open dkirkby opened 11 years ago

dkirkby commented 11 years ago

According to the guide:

The first star in the catalog is precisely centered within a pixel, and the others have random sub-pixel offsets.

However, looking at starfield_image-100-0.fits, the first star is actually centered on the stamp which, because of the even stamp size, is actually not centered in a pixel. I think the star images are fine as is, but it would be good to clarify how they are centered in the guide.

A related issue is that the corresponding catalog gives the star center as (23,23) but with the usual convention that the bottom-left corner is (1,1), the center is actually on the corner between (24,24) and (25,25). Presumably the convention here is actually 0 indexed, so that would be good to highlight (or perhaps change?)

rmandelb commented 11 years ago

Hi David -

I think I agree with you about changing the documentation regarding the star centroid, rather than changing the images. Let me just check with you: I found this incorrect statement located on https://github.com/barnabytprowe/great3-public/wiki/Challenge-dataset-format and in the handbook. Did you see it anywhere else? (I'm not asking you to go searching, but if those places are not where you saw it, please let me know so I can fix it everywhere.)

Regarding the pixel convention, I'm on the fence about documenting vs. changing. @barnabytprowe , @rmjarvis , @talljimbo, any opinions?

dkirkby commented 11 years ago

No, I haven't seen this anywhere else.

TallJimbo commented 11 years ago

The FITS and FORTRAN conventions are to start at (1,1), but the C/C++ and Python (and LSST, FWIW) conventions are to start at (0,0), and that's essentially what we've adopted in GalSim. While we could change it in the GREAT3 public catalogs without changing all the GalSim code, I think consistency with GalSim is highly desirable, and I'd recommend that we just document the (0,0) convention rather than switch to (1,1). It's also almost certain that our example analysis script will be written in Python, and we'd have to do a lot of adding/subtracting by 1 to make that appear to work in (1,1)-origin coordinates.

dkirkby commented 11 years ago

GalSim internally uses 0-offset (and x-y transposed) numpy arrays but externally uses indexing from 1 in at least a few places. For example:

galsim.ImageD(48,48).bounds
BoundsI(xmin=1, xmax=48, ymin=1, ymax=48)

I think its ok to stick with 0-indexing, but since all the data is distributed in FITS files, there is some potential for confusion when people compare with coordinates in DS9, etc.

For the record, there was some earlier discussion of this at https://github.com/GalSim-developers/GalSim/issues/380#issuecomment-16714119

TallJimbo commented 11 years ago

Hmm, I wasn't aware of this internal inconsistency. I'd still like to stick with the (0,0) convention, but it sounds like it'd be worthwhile doing a sweep through GalSim and fixing this and any other places where we use a (1,1) convention at some point. Maybe not before 1.0, though.

rmjarvis commented 11 years ago

Hmm, I wasn't aware of this internal inconsistency. I'd still like to stick with the (0,0) convention, but it sounds like it'd be worthwhile doing a sweep through GalSim and fixing this and any other places where we use a (1,1) convention at some point. Maybe not before 1.0, though.

For the record, in the discussion David referenced, the default (1,1) indexing was considered a desired feature, rather than a bug. You can change this with image.setOrigin(0,0) in python or index_convention=0 in config.

As for what convention to use for the positions in the catalogs we distribute for Great3, I don't have a strong preference so long as it is documented what we mean by these numbers.

Likewise that the first star is not centered on a pixel, but rather centered on the postage stamp which means the centroid is at the corners of 4 pixels. I think it is fine to just change the documentation about this rather than recenter these stars.

rmandelb commented 11 years ago

For now I've changed the documentation on the wiki, so as to ensure the documentation is consistent with the images + catalogs.

We could easily change conventions, but (a) I don't care that the first star is not centered on a pixel, and nobody else on this thread seems to care either (my impression was that David only cared about correctness of documentation), so let's not change that unless someone has a legitimate objection; and (b) for the catalog pixel conventions, for now it helps that the documentation is more explicit (it already said pixel coordinates were zero-indexed, though now it notes that this explicitly differs from FITS conventions, and clarifies "For example, "23" means it is at the center of the first postage stamp, with 48 pixels on a side, so in a 1-indexed convention, the centroid would be at the edge between pixels 24 and 25 along the x direction."). However, I am more inclined to change this because of the point David made about comparing with 1-indexed pixel values when viewing in some typical FITS-reader like ds9.