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.

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

DM-16537: Rework exception handling and add tests. #237

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions python/lsst/pipe/tasks/makeCoaddTempExp.py
Original file line number Diff line number Diff line change
Expand Up @@ -363,19 +363,16 @@ def run(self, calexpRefList, skyInfo, visitId=0):
for calExpInd, calExpRef in enumerate(calexpRefList):
self.log.info("Processing calexp %d of %d for this Warp: id=%s",
calExpInd+1, len(calexpRefList), calExpRef.dataId)
try:
ccdId = calExpRef.get("ccdExposureId", immediate=True)
except Exception:
ccdId = calExpInd
ccdId = calExpRef.get("ccdExposureId", immediate=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

obs_comCam doesn't have a bypass_ccdExposureId, but that's not likely to survive. Your change is now requiring any new obs_package to write a bypass_ccdExposureId in order to be able to make a warp. I thought there MIGHT be some notebooks out there that make mock dataRefs.

The conversion to Gen 3 is going to move this to runDataRef anyway.

try:
# We augment the dataRef here with the tract, which is harmless for loading things
# like calexps that don't need the tract, and necessary for meas_mosaic outputs,
# like calexps that don't need the tract, and necessary for jointcal outputs,
# which do.
calExpRef = calExpRef.butlerSubset.butler.dataRef("calexp", dataId=calExpRef.dataId,
tract=skyInfo.tractInfo.getId())
calExp = self.getCalibratedExposure(calExpRef, bgSubtracted=self.config.bgSubtracted)
except Exception as e:
self.log.warn("Calexp %s not found; skipping it: %s", calExpRef.dataId, e)
except MissingExposureError as e:
self.log.warn("Skipping missing data: %s", e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no longer include dataId? I would imagine the dataId of the missing calexp useful for debugging, though such information may be obtained through other means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assumption (and implementation) here is that the raised exception contains all the necessary information. Previously, the dataId was being duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only error from getCalibratedExposure that will include the dataId are failed butler calls. Anything else goes wrong and we won't have the dataId logged.

Note: I'm moving all the Gen2 dataRef's out of run in the next month anyway.

continue

if self.config.doApplySkyCorr:
Expand Down
58 changes: 58 additions & 0 deletions tests/test_makeCoaddTempExp.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@


class GetCalibratedExposureTestCase(lsst.utils.tests.TestCase):
"""Tests of MakeCoaddTempExpTask.getCalibratedExposure()"""
def setUp(self):
np.random.seed(10)

Expand Down Expand Up @@ -120,6 +121,63 @@ def test_getCalibratedExposureJointcal(self):
self.assertEqual(result.getWcs(), self.jointcalSkyWcs)


class MakeCoaddTempExpRunTestCase(lsst.utils.tests.TestCase):
"""Tests of MakeCoaddTempExpTask.run()."""
def setUp(self):
self.config = MakeCoaddTempExpConfig()
self.task = MakeCoaddTempExpTask(self.config)
dataId = "visit=mock"
fakeDataRef = unittest.mock.NonCallableMock(lsst.daf.persistence.ButlerDataRef,
dataId=dataId,
butlerSubset=unittest.mock.Mock())
self.calexpRefList = [fakeDataRef]

bbox = lsst.geom.Box2I(lsst.geom.Point2I(0, 0), lsst.geom.Extent2I(100, 100))
self.skyInfo = unittest.mock.Mock(bbox=bbox)

self.fakeImage = lsst.afw.image.ExposureF(bbox)
self.fakeImage.getMaskedImage().set(np.nan, lsst.afw.image.Mask.getPlaneBitMask("NO_DATA"), np.inf)

target = "lsst.pipe.tasks.makeCoaddTempExp.MakeCoaddTempExpTask._prepareEmptyExposure"
preparePatch = unittest.mock.patch(target, return_value=self.fakeImage)
preparePatch.start()
self.addCleanup(preparePatch.stop)

def testGetCalibratedExposureRaisesRuntimeError(self):
"""If getCalibratedExposure() raises anything other than
MissingExposureError, it should be passed up the chain.

Previously, all Exceptions were caught, logged at `warn` level,
and then dropped.
"""
mockErr = "Mock Error!"
target = "lsst.pipe.tasks.makeCoaddTempExp.MakeCoaddTempExpTask.getCalibratedExposure"
patch = unittest.mock.patch(target,
new_callable=unittest.mock.Mock,
side_effect=RuntimeError(mockErr))
patch.start()
self.addCleanup(patch.stop)
with self.assertRaises(RuntimeError) as cm:
self.task.run(self.calexpRefList, self.skyInfo)
self.assertIn(mockErr, str(cm.exception))

def testGetCalibratedExposureRaisesMissingExposureError(self):
"""If getCalibratedExposure() raises MissingExposureError,
processing should continue uninterrupted.
In this case, that means no data is returned, because there is only
one dataRef available (`self.fakeImage`).
"""
mockErr = "No data files exist."
target = "lsst.pipe.tasks.makeCoaddTempExp.MakeCoaddTempExpTask.getCalibratedExposure"
patch = unittest.mock.patch(target,
new_callable=unittest.mock.Mock,
side_effect=MissingExposureError(mockErr))
patch.start()
self.addCleanup(patch.stop)
result = self.task.run(self.calexpRefList, self.skyInfo)
self.assertEqual(result.exposures, {"direct": None})


def setup_module(module):
lsst.utils.tests.init()

Expand Down