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
-
- To Do
-
- Done
-
- Done
-
- Done
- relates to
-
- Done
Activity
The proposed _averagePosition seems a bit error-prone – how will you enforce the initialization requirement? It might be better to remove the default definition of getAveragePosition, even if that leads to some code duplication.
My thought was to add a constructor argument to Psf::Psf. But just removing the default implementation of Psf::getAveragePosition() probably is simpler. I'm happy to go with that.
Good point, a mandatory constructor argument would work as well. Would it still be necessary for getAveragePosition to be overrideable then?
Krzysztof Findeisen
added a comment - Good point, a mandatory constructor argument would work as well. Would it still be necessary for getAveragePosition to be overrideable then? Good point, a mandatory constructor argument would work as well. Would it still be necessary for getAveragePosition to be overrideable then?
As far as I can tell, in this case it'd be sufficient for getAveragePosition to just return _averagePosition (and not be overridable).
The trickiest bit might be adjusting the code in CoaddPsf::computeAveragePosition such that it can be used in an initializer list for _averagePosition. Or we just initialize in the ctor body I guess, and leave _averagePosition non-const, though I like the idea of making it const if possible.
I 1000% endorse this RFC for deprecating the no-position-argument interface for these methods.
I have no opinion on the exact implementation of the average position computation.
Escalating since this is an API change.
Adopting without committing to a particular implementation for getAveragePosition(). The important bit is that the default implementation is removed so that derived classes don't accidentally inherit it.
I was recently bitten by this when developing GAaP, partly because I didn't realize that the average position can be outside the bounding box if I get a small postage stamp from a coadd. Full support from me to mandate a position argument.
The proposed _averagePosition seems a bit error-prone – how will you enforce the initialization requirement? It might be better to remove the default definition of getAveragePosition, even if that leads to some code duplication.