DM-46852: Make detectAndMeasure robust to invalid PSF sigma #352

isullivan · 2024-11-05T05:40:30Z

No description provided.

parejkoj

I'm not a fan of using a default psf size, since that could produce secretly bad results, without any obvious way to know that they are bad. UpstreamFailureNoWorkFound was made for this case.

parejkoj · 2024-11-07T19:40:31Z

python/lsst/ip/diffim/detectAndMeasure.py

+            sigma = self.config.fallbackPsfSigma
+            self.log.warning("Could not determine average width of the difference image PSF. "
+                             "Using the default sigma of %d", self.config.fallbackPsfSigma)


Why can't we make this be an UpstreamFailureNoWorkFound, and do it as the first step of this method? See DM-46948 for details, but this case is exactly what that exception was made for.

Not having a useable PSF on the diffim seems like a totally reasonable failure condition. I don't like using some default psf size, since that will result in unreliable measurements and we're not doing anything to mark the output catalog as being untrustworthy.

I went back and forth on this, so I am fine checking for a valid PSF here and raising an error. I will look into UpstreamFailureNoWorkFound

parejkoj · 2024-11-07T19:43:05Z

python/lsst/ip/diffim/detectAndMeasure.py

+    clearMaskPlanes = lsst.pex.config.ListField(
+        dtype=str,
+        doc="Mask planes to clear before running detection.",
+        default=("DETECTED", "DETECTED_NEGATIVE", "NOT_DEBLENDED", "STREAK"),


Just to check: these fields are set because they were propagated from the science image?

parejkoj · 2024-11-07T19:46:25Z

python/lsst/ip/diffim/detectAndMeasure.py

-                mask.addMaskPlane(mp)
-        mask &= ~mask.getPlaneBitMask(clearMaskPlanes)
+        sigma = self._prepareInputs(difference)
+        self.log.info("Using PSF sigma of %f to smooth the difference image for detection.", sigma)


This new log is worth keeping, but I think it should go into _prepareInputs, since you forgot to put this log after the other place and there's no need to pass sigma to detection since it will get it from the (now guaranteed valid) psf.

I purposely left it out of _prepareInputs, because that is used by both regular diffim and preconvolution where it is incorrect (since we don't smooth the difference image for detection then)

Hmmm... rethinking whether the log is worth it. Returning something just to emit a log message isn't the best. Maybe that log isn't actually necessary if we only ever used the included PSF? What would be the use case of such a log message?

parejkoj · 2024-11-07T19:46:53Z

python/lsst/ip/diffim/imageDecorrelation.py

@@ -274,7 +274,7 @@ def run(self, scienceExposure, templateExposure, subtractedExposure, psfMatching
            corr = self.computeDiffimCorrection(kArr, varianceMean, targetVarianceMean)

        correctedImage = self.computeCorrectedImage(corr.corrft, subtractedExposure.image.array)
-        correctedPsf = self.computeCorrectedDiffimPsf(corr.corrft, psfImg.array)
+        # correctedPsf = self.computeCorrectedDiffimPsf(corr.corrft, psfImg.array)


This, and the other commented line below, should either be deleted, or have a TODO attached to them with the ticket number where they will be restored (or deleted).

parejkoj · 2024-11-07T19:54:43Z

python/lsst/ip/diffim/detectAndMeasure.py

+        sigma : `float`
+            The width of the PSF of the difference image.


As noted elsewhere, I think you can remove this return if you just log that info message at the end of this method.

parejkoj

Oh, and please add a test case with a garbage PSF, to check that the correct exception is raised.

Relatedly: where does the psf==None case get tested for and rejected?

isullivan · 2024-11-08T01:31:40Z

I removed the default PSF size and the related sigma logging, and instead raise an error if the width is invalid. There is a new unit test for that case, as well.

There are no unit tests of subtractImages._applyExternalCalibrations, which is where a test of the case of the external PSF being None would be relevant. Adding that test seems beyond the scope of this ticket.

parejkoj

Handful of new questions, but this is better, thanks.

parejkoj · 2024-11-08T21:53:42Z

python/lsst/ip/diffim/detectAndMeasure.py

+        Raises
+        ------
+        pipeBase.UpstreamFailureNoWorkFound
+            If the PSF is not usable for measurement


Suggested change

If the PSF is not usable for measurement

If the PSF is not usable for measurement.

parejkoj · 2024-11-08T21:54:17Z

python/lsst/ip/diffim/detectAndMeasure.py

+
+        Raises
+        ------
+        pipeBase.UpstreamFailureNoWorkFound


I think this has to be like this in docs?

Suggested change

pipeBase.UpstreamFailureNoWorkFound

lsst.pipe.base.UpstreamFailureNoWorkFound

parejkoj · 2024-11-08T21:55:06Z

python/lsst/ip/diffim/detectAndMeasure.py

+        if np.isnan(sigma):
+            raise pipeBase.UpstreamFailureNoWorkFound("Invalid PSF detected! PSF width evaluates to NaN.")


I'd put this as the first line of the method, since there's no point in doing the rest if this is raised.

parejkoj · 2024-11-08T21:56:27Z

tests/test_detectAndMeasure.py

+        # Run detection and check the results
+        with self.assertRaises(UpstreamFailureNoWorkFound):
+            detectionTask.run(science, matchedTemplate, difference,
+                              idFactory=self.idGenerator.make_table_id_factory())


No need to pass an idFactory unless you specifically care about the ids that are returned (which there won't be, since it raised).

parejkoj · 2024-11-08T21:57:41Z

tests/test_detectAndMeasure.py

+        psfNew = rng.randn(psfShape[1], psfShape[0])
+        psfcI.array = psfNew


Is this guaranteed to always make a psf that will fail the tests in the Task? Would setting all the pixels to zero or -1 be just as good, and more reliable? I know you set the seed above, but I don't think that's guaranteed to produce the same results across all architectures, is it?

You can't build a kernel with all zeros, and setting to a constant value doesn't actually raise an error. Running several different noise realizations reveals that that sometimes does result in a kernel that has a valid sigma and doesn't raise. I will instead build a fixed kernel I know should raise.

parejkoj · 2024-11-08T23:50:04Z

tests/test_detectAndMeasure.py

+        psfNew = np.zeros(psfShape)
+        psfNew[0, :] = 1


Oh, yes, that's nice and pathological!

This is not implemented correctly, and raises occasional errors.

isullivan force-pushed the tickets/DM-46852 branch from f3c8950 to 42483b5 Compare November 5, 2024 23:34

parejkoj requested changes Nov 7, 2024

View reviewed changes

parejkoj reviewed Nov 7, 2024

View reviewed changes

isullivan force-pushed the tickets/DM-46852 branch 2 times, most recently from 108bbb1 to d5b8666 Compare November 8, 2024 01:29

parejkoj approved these changes Nov 8, 2024

View reviewed changes

isullivan force-pushed the tickets/DM-46852 branch from d5b8666 to e136d38 Compare November 8, 2024 22:18

parejkoj reviewed Nov 8, 2024

View reviewed changes

isullivan added 3 commits November 8, 2024 18:07

Move common code to _prepareInputs method

7ae9d0c

Do not decorrelate the diffim PSF for now.

03400bc

This is not implemented correctly, and raises occasional errors.

Add unit test for bad PSF

b991cbf

isullivan force-pushed the tickets/DM-46852 branch from e136d38 to b991cbf Compare November 9, 2024 02:08

isullivan merged commit d3624f5 into main Nov 9, 2024
2 checks passed

isullivan deleted the tickets/DM-46852 branch November 9, 2024 03:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-46852: Make detectAndMeasure robust to invalid PSF sigma #352

DM-46852: Make detectAndMeasure robust to invalid PSF sigma #352

isullivan commented Nov 5, 2024

parejkoj left a comment

parejkoj Nov 7, 2024

isullivan Nov 7, 2024

parejkoj Nov 7, 2024

isullivan Nov 8, 2024

parejkoj Nov 7, 2024

isullivan Nov 7, 2024

parejkoj Nov 7, 2024 •

edited

Loading

parejkoj Nov 7, 2024

parejkoj Nov 7, 2024

parejkoj left a comment

isullivan commented Nov 8, 2024

parejkoj left a comment

parejkoj Nov 8, 2024

parejkoj Nov 8, 2024

parejkoj Nov 8, 2024

parejkoj Nov 8, 2024

parejkoj Nov 8, 2024

isullivan Nov 8, 2024

parejkoj Nov 8, 2024

		sigma : `float`
		The width of the PSF of the difference image.

	If the PSF is not usable for measurement
	If the PSF is not usable for measurement.

	pipeBase.UpstreamFailureNoWorkFound
	lsst.pipe.base.UpstreamFailureNoWorkFound

		if np.isnan(sigma):
		raise pipeBase.UpstreamFailureNoWorkFound("Invalid PSF detected! PSF width evaluates to NaN.")

		psfNew = rng.randn(psfShape[1], psfShape[0])
		psfcI.array = psfNew

DM-46852: Make detectAndMeasure robust to invalid PSF sigma #352

DM-46852: Make detectAndMeasure robust to invalid PSF sigma #352

Conversation

isullivan commented Nov 5, 2024

parejkoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj left a comment

Choose a reason for hiding this comment

isullivan commented Nov 8, 2024

parejkoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj Nov 7, 2024 •

edited

Loading