Extending ePSF building framework to accept odd oversampling factors and non-zero offset grid points

[Onoddil]

This PR extends EPSFBuilder and EPSFModel to accept more than the currently assumed even oversampling with grid points representing the left/right hand edges and center of each given pixel (and other linearly spaced grid points, for oversampling > 2), allowing for odd oversampling factors (including 1), and an arbitrary grid offset, displacing the first grid point from the left hand edge of a pixel, towards the right of a given pixel.

This adds the argument grid_offset representing this right hand shift of all grid points (representing the evaluation of a PRF with star offset from its central pixel by some dx, dy position), allowing for more flexible ePSF creation. Previously, the assumption was that even oversampling and zero offset meant that grid points [0, 1, 2, 3, [4]] mapped to [0, 1/4, 1/2, 3/4, [1]] detector pixel offsets (where [1] is the 0 of the next-but-one pixel) -- now grid point indices can map [0, 1, 2] to dxs of [0.1, 1/3+0.1, 2/3+0.1] for oversampling=3 and grid_offset=0.1.

This primarily is necessary to allow for oversampling=1, as otherwise each single grid point per detector pixel would have been assumed to sit in a corner of a pixel, whereas most use cases would want the grid point to represent the center of a pixel. Thus, the default inputs are 0.5 offset for oversampling=1, or no offset otherwise.

centroid_epsf was extended to accept the grid_offset argument too, as it is now necessary to check whether this algorithm can be used with the combination of oversampling and grid_offset -- this is because the algorithm, tuned to the previous defaults of oversampling=4 and grid_offset=0 from Anderson & King (2000), requires a grid point representing the center of the pixel, such that N grid points either side of this central point are equally spaced, symmetrically, either side of the middle. This symmetry evaluation is how centroid_epsf centers its data input, so this is fundamental. Thus centroid_epsf can only be used for even oversampling with zero offset, OR odd oversampling with an offset of half a grid spacing (i.e., grid_offset=0.5/oversampling). Offline discussions with @eteq resulted in the decision to keep backwards compatibility as much as possible, so centroid_epsf has been left as the default input, but I've been back and forth on that so if anyone has strong opinions the other way let me know. If all defaults are input I believe centroid_epsf should be fine (oversampling=4 and grid_offset=0 would be set by default), which makes it slightly less precarious...

Two outstanding issues I haven't dealt with (or am unsure of whether I need to deal with) are:

1. Does this change need to go anywhere in narrative documentation?
2. I haven't touched the changelog yet, primarily because of the milestone assignment (presumably this is 0.8 not being bug fixes?) but also because I'm not sure what of all of this needs to go in the changelog: grid_offset added to the three functions is API change, and I'd guess the extension of odd oversampling + arbitrary grid offsets is "New Features", but let me know what else needs to be added to the changelog and I'll add a quick commit!

cc @eteq @larrybradley for review

[pep8speaks]

Hello @Onoddil! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-21 23:30:00 UTC

[eteq]

After a quick look (will need to do more later), this is in the right direction. One thing to change, though:

• After a bit of discussion (and diagrams...) with @Onoddil, I think the default grid_offset should be 'center', which means "whatever number it needs to be for one of the epsf "pixels" to be on the center of the detector pixel". That's just 0 for even oversampling, but non-0 for odd oversampling. But I think it's the most common desire from the user, so it's best to treat it as the default even though it means the grid_offset keyword has to have an "either a number or one particular special string" as its valid inputs.
• Will want to do Removal of shift_val from EPSFModel and EPSFBuilder #995 to clean up the API, as shift_val is a rather confusing/ambiguous name for a keyword now that there's also a grid_offset. But that's probably better as a follow-on since it can be done separately from this change.

Does this change need to go anywhere in narrative documentation?

Not strictly necessary, but I think it would be useful to add an example of this to the bottom of https://photutils.readthedocs.io/en/stable/epsf.html - perhaps even follow what went above with only a change to oversampling=1 ? Obviously the PSF won't look as good but it'll at least illustrate the idea that this is now an option.

I haven't touched the changelog yet, ...

Yes, I think this goes in 0.8 and have milestoned as such. It's a bit debatable if this is a "new feature" vs "bringing back an old feature", but since the latter isn't a category I think it's best to just say it's a new feature (it's new relative to 0.7.x, anyway...).

[Onoddil]

Hi @eteq I've made the changes you suggested -- added changelog (again, let me know if there's anything I missed or if something's in the wrong place), added the "center" default for grid_offset (and extended testing for that a bit), and added a brief bit at the end of the narrative documentation to discuss odd oversampling factors and the grid offsets.

Let me know your thoughts on a more thorough test!

(cc @larrybradley also to keep in the loop)

[Onoddil]

Travis CI seems to have fallen over 500 error style, pretty sure it's not my fault :(

[larrybradley]

This was added back in version 1.0.0 (#1076) without needing an extra parameter. Forgot to close this one.