DM-54625: Update preconvolution with recent diffim improvements by isullivan · Pull Request #453 · lsst/ip_diffim

isullivan · 2026-04-10T21:17:31Z

No description provided.

kfindeisen

As forewarned, lots of comments. Some of the factoring I'm asking for is pre-existing and should probably be deferred to a later work item, but I suggest at least streamlining the "when to do CR removal vs. background removal" logic. And I really don't understand how the Diff_scoreExp vs. Diff_scoreTempExp swap doesn't break things.

kfindeisen · 2026-04-29T19:54:59Z

        x0 = 123
        y0 = 456
+        kernelSize = 31
+        templateBorderSize = kernelSize//2


Again, if you're going to have a comment for one copy of the code, you probably need it everywhere. (Can the copies be factored out?)

isullivan · 2026-05-04T02:59:43Z

This is ready for another round of review. The commits that start "fixup" are intended to be squashed into the referenced commit. I left them pre-rebasing so that the changes are visible.

Also make sure to use a consistent PSF size for preconvolution and decorrelation.

kfindeisen

Mostly minor comments, but I'm still concerned about the change in role for *Diff_scoreExp -- something you said was a bug earlier? If it is, it would affect lsst/ap_pipe#269 as well.

kfindeisen · 2026-05-04T19:28:20Z

+            # the size was auto-computed. An explicit ``scienceFwhmPix`` is
+            # assumed already to reflect the preconvolved effective width.
+            if preconvolved:
+                scienceFwhmPix *= np.sqrt(2)


This code runs if scienceFwhmPix was passed in but templateFwhmPix was None. Was that the intent? Same with the indented block on lines 213-214 and the version moved into _checkPsfWidths.

Yes, that was intentional. Both scienceFwhmPix and templateFwhmPix have to be specified or else both are recalculated, and in that case the preconvolved check needs to be made.

kfindeisen · 2026-05-04T20:39:55Z

+            # Background is measured on the difference image, and then
+            # subtracted from both the difference and score images.
+            # The preconvolution kernel is normalized to 1, so the
+            # same background level applies to both.
+            background = self.subtractInitialBackground.run(difference.clone()).background
+            detectionScoreExposure = scoreExposure.clone()
+            detectionScoreExposure.image -= background.getImage()


I still think it's confusing to talk about subtracting the background from the difference image when you're going out of your way to not do that. Maybe just have the last two lines (with "both difference and score images")?

kfindeisen · 2026-05-04T21:55:49Z

        _detection_wrapper(positive=True)
        _detection_wrapper(positive=False)

+    def test_mask_cosmic_rays(self):


Redefinition of method from line 417.

It is not obvious, but this is a different version than the first one. I'll update the docstring to mention the difference.

kfindeisen · 2026-05-04T22:48:54Z

-            backgroundModel.setParameters([-p for p in modelParams])
+            invertedBackground = backgroundModel.clone()
+            invertedBackground.setParameters([-p for p in backgroundModel.getParameters()])
+            backgroundModel = invertedBackground


Could you expand the commit message to mention that this is for legibility? Right now it just says "Clone the background model before inverting", which implies that there's some obvious(?) reason why you have to do that (e.g., a bug fix).

kfindeisen · 2026-05-04T22:49:23Z

-                maglim = np.nan
+                return np.nan
+            self.log.info("Unable to evaluate PSF, using fallback FWHM %f", fallbackPsfSize)
+            psf_area = np.pi*(fallbackPsfSize/2)**2


Could you mention the return-from-finally in the commit message, so that it's clearer what's going on?

kfindeisen · 2026-05-04T23:54:26Z

        self._prepareInputs(template, science, visitSummary=visitSummary)

        convolveTemplate = self.chooseConvolutionMethod(template, science)
+        self.matchedPsfSize = self.sciencePsfSize if convolveTemplate else self.templatePsfSize


Should the commit message read "exactly once" instead of "only once"?

kfindeisen · 2026-05-05T00:06:10Z

                Sources from the input catalog that were used to construct the
                PSF-matching kernel.
        """
+        self.usePreconvolution = False


Something seems fishy here. The only references that set usePreconvolution are:

AlardLuptonSubtractTask.run sets self.usePreconvolution = False

AlardLuptonPreconvolveSubtractTask.run sets self.usePreconvolution = True

SimplifiedSubtractTask.run sets self.usePreconvolution = False

If the goal is to define usePreconvolution as a function of the implementation class instead of the arguments, then it would be better to set as a class constant, where at least it's clear that there won't be any side effects or weird interactions (e.g., what if AlardLuptonPreconvolveSubtractTask.run called super().run(), unlikely as it is?). It also avoids any potential problems from usePreconvolution being referenced before run is called.

(TBH, making this a class variable/constant still feels wrong, but I can't come up with a better idea at the moment.)

I've moved usePreconvolution to a class variable, and checked that it is used instead of other flags where possible.

kfindeisen · 2026-05-05T00:07:14Z

        """
-
+        if self.usePreconvolution:
+            raise RuntimeError("Incorrect matching kernel calculation configured.")


Be more specific about how it's incorrect?

kfindeisen · 2026-05-05T00:09:08Z

-        self.preconvolvedSciencePsfSize = self.sciencePsfSize*np.sqrt(2)
-        self.log.info("Preconvolved science PSF FWHM: %f pixels", self.preconvolvedSciencePsfSize)
-        self.metadata["preconvolvedSciencePsfSize"] = self.preconvolvedSciencePsfSize
+        self.matchedPsfSize = self.sciencePsfSize*np.sqrt(2)


Duplicate of line 1228 (though I think this spot is better, since it's immediately before first use).

kfindeisen · 2026-05-05T00:39:55Z

-        if self.config.doSubtractBackground:
-            # Run background subtraction before clearing the mask planes
-            detectionExposure = difference.clone()
-            background = self.subtractInitialBackground.run(detectionExposure).background
-        else:
-            detectionExposure = difference
-            background = afwMath.BackgroundList()
+        # Run background subtraction (if configured) before clearing the mask
+        # planes.
+        detectionExposure, background = self._subtractInitialBackground(differenceExposure=difference)

        self._prepareInputs(detectionExposure)

-        if self.config.doFindCosmicRays and not self.config.doSubtractBackground:
-            # Detect and interpolate over any remaining cosmic rays
-            # This only needs to run once, right before the final detection.
-            self.findAndMaskCosmicRays(detectionExposure)
+        # Detect and interpolate over any remaining cosmic rays. When
+        # background subtraction is enabled, this is deferred to the second
+        # pass so it runs only once on a properly background-subtracted image.
+        self._findInitialCosmicRays(differenceExposure=difference)


This isn't what I had in mind; I thought it would be simpler to have something like:

# Preprocess or skip if doSubtractBackground: detectionExposure = copy background = rough background self._prepareInputs(detectionExposure) # Only run on first pass, but not second? results = detection.run(detectionExposure) background = final background else: background = empty self._prepareInputs(difference) # Run on only pass # Final processing self.findAndMaskCosmicRays(difference) # current code aliases as detectionExposure if not doSubtractBackground results = detection.run(difference)

What you're proposing here obfuscates the multiple doSubtractBackground checks instead of eliminating them.

I've pushed a new version that refactors background subtraction and cosmic ray masking.

kfindeisen

With the connections question resolved, I have no major objections.

Otherwise the results are unstable when the science image has a psfSize of 2.0 in the unit tests and the template has a psfSize of 2.3 to 2.4. This change makes the basis a smooth transition instead of a sudden jump.

…ault (0,0)

Mostly for readability, but this does prevent modifying the original background model if the function is called in a notebook environment.

The return statement inside the finally block is bad practice and suppressed error messages.

This does not suffer from the same bug as setting the image plane to 0 for masked pixels, since the peak is removed after detection.

Consolidate setting common values, but there should be no behavior changes.

Also remove the redundant calculation of the kernel source list in subtractImages - makeCandidateList was effectively called twice.

Use common code for both standard and Score versions.

isullivan force-pushed the tickets/DM-54625 branch from 1516e2a to a51832a Compare April 20, 2026 22:17

isullivan changed the title ~~DM-54625: Support partial outputs for detection on the preconvolved diffim~~ DM-54625: Update preconvolution with recent diffim improvements Apr 22, 2026

isullivan force-pushed the tickets/DM-54625 branch from 3a48aa0 to 8b113d3 Compare April 22, 2026 23:29

Support partial outputs for detection on the preconvolved diffim

4de2803

isullivan force-pushed the tickets/DM-54625 branch 4 times, most recently from 8a2e2ab to cd5766b Compare April 27, 2026 23:54

kfindeisen requested changes Apr 29, 2026

View reviewed changes

isullivan added 2 commits May 4, 2026 12:21

Reflect the science image PSF for preconvolution.

cec11ba

Also make sure to use a consistent PSF size for preconvolution and decorrelation.

Raise if the kernel is not odd-sized

23f97db

isullivan force-pushed the tickets/DM-54625 branch from 069011b to 930510f Compare May 4, 2026 19:24

kfindeisen requested changes May 5, 2026

View reviewed changes

kfindeisen approved these changes May 5, 2026

View reviewed changes

isullivan added 12 commits May 5, 2026 13:54

Ensure that the border of the Score image has EDGE mask set

95bb28a

Add background subtraction for preconvolution

a03d1cf

Add cosmic ray detection for preconvolution

10809b8

The matchedTemplate should not be scaled a second time

88b9c9e

Reduce PSF size in tests to keep PSF fully contained in kernel size

1a86010

Fix discontinuity in kernel basis

9c15ad4

Otherwise the results are unstable when the science image has a psfSize of 2.0 in the unit tests and the template has a psfSize of 2.3 to 2.4. This change makes the basis a smooth transition instead of a sudden jump.

Fix broken error message

4a5cf7f

Evaluate psf matching kernel norm in the center of the image, not def…

b24f20d

…ault (0,0)

Clone the background model before inverting

abfe16e

Mostly for readability, but this does prevent modifying the original background model if the function is called in a notebook environment.

Errors in calculating limiting magnitudes were being silently squashed

0ea8616

The return statement inside the finally block is bad practice and suppressed error messages.

Add more details to setSourceFootprints docstring

c627e84

Persist the Score image post-detection and BG subtraction

be3e03f

isullivan force-pushed the tickets/DM-54625 branch 2 times, most recently from 0faf38c to 229d253 Compare May 6, 2026 18:32

Add option to reject bad peaks prior to measurement.

88ff554

This does not suffer from the same bug as setting the image plane to 0 for masked pixels, since the peak is removed after detection.

isullivan added 6 commits May 6, 2026 14:49

Clean up diffim unit tests

0bb7c22

Consolidate setting common values, but there should be no behavior changes.

Pass keywords by name to kernel.computeImage

1dfae58

Move duplicated PSF width checks in makeKernel to common method

c69f2db

Also remove the redundant calculation of the kernel source list in subtractImages - makeCandidateList was effectively called twice.

Test a range of psf sizes in equal images unit test

36dec17

Refactor psfSize calculations to make sure it is done exactly once

9cca0d7

Refactor CR detection and background subtraction

610e70c

Use common code for both standard and Score versions.

isullivan force-pushed the tickets/DM-54625 branch from 229d253 to 610e70c Compare May 6, 2026 21:52

isullivan merged commit 3dd0db4 into main May 6, 2026
3 of 4 checks passed

isullivan deleted the tickets/DM-54625 branch May 6, 2026 22:45

Conversation

isullivan commented Apr 10, 2026

Uh oh!

kfindeisen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

isullivan commented May 4, 2026

Uh oh!

kfindeisen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfindeisen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants