Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DM-39842: Convert MeasureApCorr to new exceptions #364

Merged
merged 1 commit into from
Mar 22, 2024
Merged

DM-39842: Convert MeasureApCorr to new exceptions #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 44 additions & 13 deletions python/lsst/meas/algorithms/measureApCorr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,50 @@
import lsst.pex.config
from lsst.afw.image import ApCorrMap
from lsst.afw.math import ChebyshevBoundedField, ChebyshevBoundedFieldConfig
from lsst.pipe.base import Task, Struct
from lsst.pipe.base import Task, Struct, AlgorithmError
from lsst.meas.base.apCorrRegistry import getApCorrNameSet

from .sourceSelector import sourceSelectorRegistry


class MeasureApCorrError(RuntimeError):
pass
class MeasureApCorrError(AlgorithmError):
"""Raised if Aperture Correction fails in a non-recoverable way.

Parameters
----------
name : `str`
Name of the kind of aperture correction that failed; typically an
instFlux catalog field.
nSources : `int`
Number of sources available to the fitter at the point of failure.
ndof : `int`
Number of degrees of freedom required at the point of failure.
iteration : `int`, optional
Which fit iteration the failure occurred in.
"""
def __init__(self, *, name, nSources, ndof, iteration=None):
msg = f"Unable to measure aperture correction for '{name}'"
if iteration is not None:
msg += f" after {iteration} steps:"
else:
msg += ":"
msg += f" only {nSources} sources, but require at least {ndof}."
super().__init__(msg)
self.name = name
self.nSources = nSources
self.ndof = ndof
self.iteration = iteration

@property
def metadata(self):
metadata = {"name": self.name,
"nSources": self.nSources,
"ndof": self.ndof,
}
# NOTE: have to do this because task metadata doesn't allow None.
if self.iteration is not None:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the is not None part needed here? I feel like flake8 has yelled at me in the past for not just doing, e.g., if thing instead of if thing is not None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're thinking of if thing == None, which is not good (None is a singleton). In this case, the check is explicitly "is this None", not any other "falsey" value.

metadata["iteration"] = self.iteration
return metadata


class _FluxNames:
Expand Down Expand Up @@ -232,11 +268,8 @@ def run(self, exposure, catalog):
name, isGood.sum(), self.config.minDegreesOfFreedom + 1)
continue
else:
msg = ("Unable to measure aperture correction for required algorithm '%s': "
"only %d sources, but require at least %d." %
(name, isGood.sum(), self.config.minDegreesOfFreedom + 1))
self.log.warning(msg)
raise MeasureApCorrError("Aperture correction failed on required algorithm.")
raise MeasureApCorrError(name=name, nSources=isGood.sum(),
ndof=self.config.minDegreesOfFreedom + 1)

goodCat = goodRefCat[isGood].copy()

Expand Down Expand Up @@ -286,11 +319,9 @@ def run(self, exposure, catalog):
(name, keep.sum(), self.config.minDegreesOfFreedom + 1))
break
else:
msg = ("Unable to measure aperture correction for required algorithm "
"'%s': only %d sources remain, but require at least %d." %
(name, keep.sum(), self.config.minDegreesOfFreedom + 1))
self.log.warning(msg)
raise MeasureApCorrError("Aperture correction failed on required algorithm.")
raise MeasureApCorrError(name=name, nSources=keep.sum(),
ndof=self.config.minDegreesOfFreedom + 1,
iteration=iteration+1)

apCorrField = ChebyshevBoundedField.fit(bbox, x, y, z, ctrl)
fitValues = apCorrField.evaluate(x, y)
Expand Down
21 changes: 11 additions & 10 deletions tests/test_measureApCorr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,16 +153,18 @@ def testTooFewSources(self):
""" If there are too few sources, check that an exception is raised."""
# Create an empty catalog with no sources to process.
catalog = afwTable.SourceCatalog(self.schema)
with self.assertLogs(level=logging.WARNING) as cm:
with self.assertRaisesRegex(measureApCorr.MeasureApCorrError, "failed on required algorithm"):
self.meas_apCorr_task.run(catalog=catalog, exposure=self.exposure)
self.assertIn("Unable to measure aperture correction for required algorithm", cm.output[0])
with self.assertRaisesRegex(measureApCorr.MeasureApCorrError,
"Unable to measure aperture correction for 'test1Ap'"):
self.meas_apCorr_task.run(catalog=catalog, exposure=self.exposure)

# We now try again after declaring that the aperture correction is
# allowed to fail. This should run cleanly without raising an exception.
# allowed to fail. This should run without raising an exception, but
# will log a warning.
Comment on lines 160 to +162
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really nice example of the utility of this new exception handling option! However, it's not clear to me how a user would set this at runtime, do you have an example for how to do that available somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specifically for the aperture correction algorithm: allowFailure is the config option that sets which measurements are allowed to fail. It's a feature that DRP uses, but I don't know anything about it. I'm just making the existing tests support the new exception system.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I know this is a specific situation where you now use allowFailure, and thus here lies the one place where all of this began to make sense to me. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, allowFailure has always been in MeasureApCorr, it has nothing to do with my changes. All the change here does is make the test pass, since the details of the exception have changed.

for name in self.names:
self.meas_apCorr_task.config.allowFailure.append(name + self.apNameStr)
self.meas_apCorr_task.run(catalog=catalog, exposure=self.exposure)
with self.assertLogs(level=logging.WARNING) as cm:
self.meas_apCorr_task.run(catalog=catalog, exposure=self.exposure)
self.assertIn("Unable to measure aperture correction for 'test1Ap'", cm.output[0])

def testSourceNotUsed(self):
""" Check that a source outside the bounding box is flagged as not used (False)."""
Expand Down Expand Up @@ -247,10 +249,9 @@ def testTooFewSourcesAfterFiltering(self):
nameAp = name + self.apNameStr
sourceCat[nameAp + "_instFlux"][0] = 100.0

with self.assertLogs(level=logging.WARNING) as cm:
with self.assertRaisesRegex(measureApCorr.MeasureApCorrError, "Aperture correction failed"):
self.meas_apCorr_task.run(catalog=sourceCat, exposure=self.exposure)
self.assertIn("only 4 sources remain", cm.output[0])
with self.assertRaisesRegex(measureApCorr.MeasureApCorrError,
f"Unable to measure aperture correction for '{nameAp}'"):
self.meas_apCorr_task.run(catalog=sourceCat, exposure=self.exposure)

# We now try again after declaring that the aperture correction is
# allowed to fail. This should run cleanly without raising an exception.
Expand Down