Details
-
Type:
RFC
-
Status: Adopted
-
Resolution: Unresolved
-
Component/s: DM
-
Labels:None
Description
Most of the Psf methods that can compute something at a particular point in an image allow for the actual position argument to be omitted, in which case an "average" position is used by default. See many of the methods in Psf.h This pattern can easily lead to problems when either:
- the programmer forgets to specify a position (see https://github.com/lsst/meas_extensions_gaap/pull/19/files#diff-a2b9e86cf219f9d08340e128bd919d674908cb94a15eeb8231c4936e544df26eL121)
- the programmer assumes that the computation is independent of position (https://github.com/lsst/meas_extensions_shapeHSM/pull/33/files#diff-ded2fd089f49b8ea4563875ff5d65ca1b978c723e9d35bc5d38c726c6419dd47L200).
In cases where the position really is intended to be the average position, it's easy enough to query the Psf object that the programmer already has access to, e.g.:
psf.computeImage(psf.getAveragePosition())
Since this pattern appears to be dangerous and unnecessary, I propose to deprecate the no-position-argument interface in each of:
Psf::computeImage
Psf::computeKernelImage
Psf::computePeak
Psf::computeApertureFlux
Psf::computeShape
Psf::getLocalKernel
Psf::computeBBox
Psf::computeImageBBox
Psf::computeKernelBBox
and their python equivalents.
(I haven't tried, but I'm guessing we could implement this in c++ by swapping the default parametered methods for overloads).
Second, Psf::getAveragePosition() has a virtual default implementation, which just returns Point2D(). It is up to derived classes to override this if required, which should be the case for all non-constant Psf models. However, I'm pretty sure I didn't do this right in meas_extensions_piff. So part (b) of this RFC is to add a Psf::_averagePosition member variable and require constructors to initialize it. This is effectively already done in a number of derived classes, e.g.,
- https://github.com/lsst/meas_algorithms/blob/master/include/lsst/meas/algorithms/KernelPsf.h#L48
- https://github.com/lsst/meas_algorithms/blob/master/include/lsst/meas/algorithms/PcaPsf.h#L45
- https://github.com/lsst/meas_extensions_psfex/blob/master/include/lsst/meas/extensions/psfex/PsfexPsf.h#L46
Making it mandatory will prevent any future Psf derived classes from naively inheriting a default implementation.
Attachments
Issue Links
- is triggering
-
DM-31536 Remove default implementation of Psf::getAveragePosition()
- To Do
-
DM-31535 Deprecate PSF methods default position argument
- Done
-
DM-31707 Remove deprecated Psf methods
- Done
-
DM-34770 Remove deprecated uses of psf computeShape() and friends
- Done
- relates to
-
DM-31777 Determine and use correct position for PSF shape determination in ImageDifferenceTask
- Done