Use of PSF when interpolating over defects

The function used to interpolate over a defects, interpolateOverDefects in meas_algorithms, requires a Psf input argument. However, the current implementation makes no use of this Psf (as evidenced in the function definition which does not associate it with a variable name). There are several functions throughout the codebase that make use of this function for interpolation. Some are aware of the fact that the Psf argument is not used, e.g. the documentation for interpolateOnePlane states:

Note that the interpolation code in meas_algorithms currently
doesn’t use the input PSF (though it’s a required argument),
so it’s not important to set the input PSF parameters exactly.

However, there are a host of calls to the interpolation function that are not necessarily aware that the Psf they are feeding it is not in fact being used. An example is in ip_isr’s isrTask.py which defines fwhm as a config parameter. This fwhm is then passed to several different functions whose purpose is to perform interpolation over some defect type (e.g. saturationInterpolation, maskAndInterpDefect, maskAndInterpNan). These all feed into the interpolateOverDefects function which make no use of the fwhm being passed. In another example, obs_subaru’s isr.py sets a config parameter:
fwhmForBadColumnInterpolation = pexConfig.Field(
    dtype = float,
    doc = "FWHM of PSF used when interpolating over bad columns (arcsec)",
    default = 1.0,
)

which certainly implies that it is being used to some effect.

My original instinct was to remove the unused parameter from interpolateOverDefects and adapt any calls to it in the rest of the codebase. However, given the above, I think we should be asking the question of wether the interpolation code should be making use of the Psf (estimated or not), and perhaps there should be two versions: one requiring and using the Psf and the other not (i.e. the function as is).

As it currently stands, I think it is very misleading to have so many tasks using this function and perhaps making some erroneous assumptions about the use of the Psf they are feeding into it.

I think @rhl may have some thoughts/opinions on the original intentions of including this placeholder in the function. Was/is there a plan to make use of the Psf in the interpolation that has fallen through the cracks?

I absolutely intend to use the PSF FWHM; in fact I’d prefer a PSF rather than just its FWHM. It’s an accident of the SDSS-legacy interpolator that it’s ignored (it’s a Gaussian-process code that hard-codes the autocorrelation)

Ok, great. Do you propose we continue with the Psf placeholder as is and continue to have calling functions to supply a Psf (and it is a lsst::afw::detection::Psf; calling functions have been creating one from the fwhm using, e.g. measAlg.DoubleGaussianPsf or measAlg.GaussianPsfFactory)?

Yes, I’d provide a Psf. You could accept a float and convert it internally (a little ugly, but convenient for the user)

The Psf is already being provided by all callers as it is currently a required argument. My worry here is that the callers actually believe that the Psf is being used to some effect in the interpolation. The obs_subaru example above certainly gives the impression that this PSF is “used when interpolating over bad columns”. I find this confusing and misleading.

How about we log a runtime warning that currently the PSF is not used (but may be in the future)? The documentation for the method should say as well.

I don’t think it’s a good idea to log a runtime warning that 1) occurs every time and 2) the user and developer can do nothing about. (Some of the Butler warnings fall into this category, too.) A warning in the documentation seems appropriate.

+1 on a note in the documentation for that implementation of this function. We’d need to flow this up to the Task documentation.

Added DM-4097 to JIRA to request appropriate documentation.

I don’t think we should add warnings for internal algorithmic details (we should of course add the documentation). For example, sdss.centroid doesn’t currently debias the estimated centre for skew in the PSF; yes, it could/should but we wouldn’t want to add a log message to that effect.