Joshua Meyers and I worked on this a bit in pair coding today. A potential source of the problem is that Piff PSF stamps are smaller (21x21 pixels) than PSFEx. insertFakes.py uses a default calibFluxRadius=12.0, and psf.computeApertureFlux will silently return NaN values when the radius is larger than than the stamp size (which is the case now). In turn the inserted fake magnitudes are likely to be NaN, which would explain why the fakes metrics do not find them. A workaround would be to use a ten-pixel radius by default, but we should also add some additional logging.

Oh my. 21 pixels is so close to being enough. I don't know anything about how PIFF chooses its stamps size. Is there a way we can ask PIFF to use at least 25x25?

Is there anything special about calibFluxRadius=12.0? We thought maybe just using 10.0 would be a decent workaround, rather than changing the PIFF stamp sizes.

Try 10 and see if you get an offset in the input vs output fluxes.  If not, great!

Some more information:

• What's special about 12 is that it's the aperture used for the CalibFlux:   https://github.com/lsst/meas_base/blob/26961ef6c7a3bd91c4b40a0307c740244fea775e/python/lsst/meas/base/baseMeasurement.py#L103
• 21 pixels was not motivated by anything and just what shook out with the defaults. 
• Through experience with the HSC data releases we progressively made the PSF models larger, and we're expecting to have to make the PIFF PSF models larger. 

To help with debugging this, I've pushed a change to characterizeImage to make the PSF estimation there fail if sigma is NaN. I think this is behavior we'll want anyway (to prevent NaN psfs from causing failures much further downstream), and I would guess it hasn't been a problem previously because pre-PIFF PSF determiners didn't output NaN PSFs.

Eric will comment more on better error handling and log messages in PIFF.

The ap_verify_ci_cosmos_pdr2 branch requires DM-34531 of meas_extensions_piff; we'll want that change on each of the ap_verify datasets once we figure out why it causes piff to crash, or we'll want to default that in ap_pipe, or we'll want to default it in piff itself.

Providing a bit of context for John's comments: We pair-coded on this today for a bit. We reconfigured Piff to use a 25-pixel kernelSize (via DM-34531). Unfortunately this did not resolve the "fake metrics are zero" problem, and in fact the pipeline failed to complete: it fails with the errors below:

{{ File "/software/lsstsw/stack_20220421/stack/miniconda3-py38_4.9.2-3.0.0/Linux64/meas_algorithms/gf36ae6ace1+1d7ae49801/python/lsst/meas/algorithms/detection.py", line 688, in detectFootprints
psf = self.getPsf(exposure, sigma=sigma)
File "/software/lsstsw/stack_20220421/stack/miniconda3-py38_4.9.2-3.0.0/Linux64/meas_algorithms/gf36ae6ace1+1d7ae49801/python/lsst/meas/algorithms/detection.py", line 411, in getPsf
size = self.calculateKernelSize(sigma)
File "/software/lsstsw/stack_20220421/stack/miniconda3-py38_4.9.2-3.0.0/Linux64/meas_algorithms/gf36ae6ace1+1d7ae49801/python/lsst/meas/algorithms/detection.py", line 386, in calculateKernelSize
return (int(sigma * self.config.nSigmaForKernel + 0.5)//2)*2 + 1 # make sure it is odd
ValueError: cannot convert float NaN to integer}}

As John mentions, this could have been caught earlier: characterizeImage logs

lsst.characterizeImage INFO: iter 1; PSF sigma=nan, dimensions=(25, 25); median background=770.85

so we should notice and catch when the PSF is providing NaNs.

This is one of several places where the Piff log levels should be adjusted;

lsst.characterizeImage.measurePsf.psfDeterminer INFO: Ill-conditioned matrix (rcond=2.40177e-26): result may not be accurate. might be more severe than INFO?

and lsst.characterizeImage.measurePsf.psfDeterminer gives WARN for total chi-squared and the start of each fitting iteration.

To summarize: I think we are stuck here and need help from Joshua Meyers to understand why the Piff PSFs are returning NaNs even after we adjust the kernel size.

Following up here, Joshua Meyers and I created a failing test case with piff to demonstrate the error independent of Rubin code (we just pickled up the star images generated in a test) and filed an issue with Piff:

https://github.com/rmjarvis/Piff/issues/131

Further followup: our psfCandidates were too small. The values of makePsfCandidates.kernelSize and psfDeterminer.kernelSize are entirely independent, and not checked against each other. PcaPsf and Psfex both force the psfCandidates to have dimensions that equal their kernelSize, but they also modify that kernelSize before fitting. I've implemented checks in piffPsfDeterminer and MeasurePsfConfig to try to prevent this. I'll put those up for review shortly, I hope.

Running ap_verify on the modified ap_verify_ci_cosmos pipeline yaml with kernelSize=25 did not crash. I'll leave it to someone else to check that this fixes the fakes metric problem described on this ticket.

I've updated the piff Issue linked above to reflect that piff should itself fail in this situation.

Jenkins: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/36498/pipeline

Nice detective work. 

And we need to override the kernel configs deeper than ap_verify_ci_hsc_pdr2 too, though I get that we need to do ap_verify_ci_hsc_pdr2 ASAP, so fine if you want to do this another ticket. 

Joshua Meyers, can we change kernelMin and kernelMax here to 25 and 45 like our other psfDeterminers?

 https://github.com/lsst/meas_extensions_piff/blob/4882acf2830983cd070aa1a906dc2fcf4ac82c04/python/lsst/meas/extensions/piff/piffPsfDeterminer.py#L75

e.g. currrent:

config.measurePsf.makePsfCandidates.kernelSize=21

config.measurePsf.psfDeterminer['pca'].kernelSize=10.0

config.measurePsf.psfDeterminer['pca'].kernelSizeMin=25

config.measurePsf.psfDeterminer['pca'].kernelSizeMax=45

config.measurePsf.psfDeterminer['psfex'].kernelSize=81

config.measurePsf.psfDeterminer['psfex'].kernelSizeMin=25

config.measurePsf.psfDeterminer['psfex'].kernelSizeMax=45 

config.measurePsf.psfDeterminer['piff'].kernelSize=21.0

config.measurePsf.psfDeterminer['piff'].kernelSizeMin=11 # not like the others

config.measurePsf.psfDeterminer['piff'].kernelSizeMax=35 # not like the others

The min/max kernelSize values are irrelevant for piffPsfDeterminer: see discussion on the PR for DM-34531 for details.

I think it worked? Here's what I get for one of the completeness metrics from the ap_verify metrics list with the kernelSize=25 change (it is 0 if I run on main):

In [12]: butler.get('metricvalue_ap_pipe_ApFakesCompletenessMag20t22', instrument='HSC', detector=50, visit=59150, collections="ap_verify-output")

Out[12]: Measurement('ap_pipe.ApFakesCompletenessMag20t22', <Quantity 0.66666667>)

Eric Bellm: there are three PRs to review. I think the biggest question is whether we need to make the same kernel size change to the hits2015 ap_verify datasets or whether we should make it to ap_verify or ap_pipe so it is used for all datasets.

I think the biggest question is whether we need to make the same kernel size change to the hits2015 ap_verify datasets or whether we should make it to ap_verify or ap_pipe so it is used for all datasets.

This kind of change does not belong in the dataset-specific configs. The only things that should go there are configurations to customize the pipeline to the contents of the datasets.

Ideally the change would be global in meas_extensions_piff. 

Changing this globally in meas_extensions_piff would affect DRP, which would require DRP to do validation runs. We'd rather get the change in sooner than that, I think. We can have a follow-up ticket to make the change in meas_extensions_piff.

Thanks, John Parejko. DRP is going to handle changing the Piff default to 25. In the meantime we should set it for all ap pipelines. Right now you have a PR only for the HSC CI dataset; we should make the config change in ap_pipe instead, and confirm that it is inherited appropriately in CI.

I left some other questions on the pipe_tasks and meas_extensions_piff PRs.

New Jenkins, with change in ap_pipe:https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/36549/pipeline

Looks good once Jenkins is available.