DM-44287: Refactor error handling to work better with measurement framework and quiet excessive logging #37
Conversation
| @@ -328,7 +328,8 @@ def _solve(self, kernelCellSet, basisList): | |||
| spatialKernel, spatialBackground = spatialkv.getSolutionPair() | |||
| spatialSolution = spatialkv.getKernelSolution() | |||
| except Exception as e: | |||
| self.log.error("ERROR: Unable to calculate psf matching kernel") | |||
| # This is just a debug log because it is caught by the GAaP plugin. | |||
| self.log.debug("ERROR: Unable to calculate psf matching kernel") | |||
There was a problem hiding this comment.
Let's drop the screaming ERROR: as well then. That should have not been there in the first place.
| @@ -562,16 +541,23 @@ def _gaussianizeAndMeasure(self, measRecord: lsst.afw.table.SourceRecord, | |||
| # scaling factors and sigmas. | |||
| if measRecord.getFootprint().getArea() == 0: | |||
| self._setFlag(measRecord, self.name, "no_pixel") | |||
| raise NoPixelError | |||
| self._setScalingAndSigmaFlags(measRecord, self.config.scalingFactors) | |||
| raise NoPixelError("No good pixels in footprint", 1) | |||
There was a problem hiding this comment.
If we are moving this around, let's move this below the next block that raises the FatalAlgorithmError. On the very off chance something went so bad in processing that all footprints have zero area and no PSF available, I'd want it to fail with a FatalAlgorithmError.
| raise GaapConvolutionError(errorCollection) | ||
| center = measRecord.getCentroid() | ||
| self.log.debug("Failed to solve for PSF matching kernel in GAaP for (%f, %f): %s", | ||
| center.getX(), center.getY(), "GAaP Convolution Error") |
There was a problem hiding this comment.
Why does the error message say fail to solve for kernel when we don't even attempt solving for it? It made sense when we were handling this in fail but we can be more specific here.
| """ | ||
| for scalingFactor in scalingFactors: | ||
| if specificFlag is not None: | ||
| flagName = self.ConfigClass._getGaapResultName(scalingFactor, "flag_gaussianization", |
There was a problem hiding this comment.
| flagName = self.ConfigClass._getGaapResultName(scalingFactor, "flag_gaussianization", | |
| flagName = self.ConfigClass._getGaapResultName(scalingFactor, specificFlag, |
| baseName = self.ConfigClass._getGaapResultName(scalingFactor, sigma, self.name) | ||
| self._setFlag(measRecord, baseName) | ||
| # We have set the flags and done appropriate logging. | ||
| pass |
There was a problem hiding this comment.
May be simplify this as
if error is None:
measRecord.set(self._failKey, True)| self.config.scalingFactors, | ||
| specificFlag="flag_gaussianization", | ||
| ) | ||
| raise GaapConvolutionError("Failed to solve for PSF matching kernel", 1) |
There was a problem hiding this comment.
I'd raise this as a plain MeasurementError and keep the error handling in GaapConvolutionError instead of trivially subclassing it from MeasurementError.
There was a problem hiding this comment.
I'm not sure what you mean. I don't see the harm in having a slightly more specific error so that if you look at the debug log there's marginally more information (though there is still the message). But I don't like having the error handling in the Error code because that's not what exceptions are supposed to do.
There was a problem hiding this comment.
Okay, if we want to keep error handling/error collating outside of GaapConvolutionError, then raising this specific type is fine.
|
There's also a reference to DM-27482 which is now marked as Done. If you could remove the comment, that'd be great! |
| self.config.scalingFactors, | ||
| specificFlag="flag_gaussianization", | ||
| ) | ||
| raise GaapConvolutionError("Failed to solve for PSF matching kernel", 1) |
There was a problem hiding this comment.
Okay, if we want to keep error handling/error collating outside of GaapConvolutionError, then raising this specific type is fine.
…ework. This moves flag handling out of the `fail` to avoid highly specific contextual requirements; improves flagging when footprints are empty; and removes unnecessary error logging.
No description provided.