DM-53757: Implement injecting into cell coadds by taranu · Pull Request #75 · lsst/source_injection

taranu · 2026-09-08T22:49:21Z

No description provided.

TallJimbo · 2026-09-09T14:28:56Z

+        # Normalize first
+        # TODO: Should also deal with negative pixel values here as they are
+        # not valid for a PSF.
+        psf_array /= np.sum(psf_array)


I believe lsst.afw.detection.Psf already does this for you.

computeKernelImage does not, and neither does compute_kernel_image in lsst.images. I checked a handful of cells in a semi-random ECDFS coadd and the sum of the PSF arrays differed from unity by O(1e-4), and the sum of the negative-valued pixels was O(1e-3), with one particular example where it was almost -0.02. But that's an issue for another ticket.

TallJimbo · 2026-09-09T14:34:49Z

+                        fallback_psf = galsim_psf
+                except BoundsError:
+                    # TODO: Should this ever happen? If it does, it probably
+                    # means that cell_coadd.bounds.missing is wrong.


AFAIK, this is indeed impossible. But it is possible to get a missing aperture correction in a cell that is not in cell_coadd.bounds.missing.

Should I try to catch a different/broader exception, then?

I lean towards not having a try/except block at all when I don't know the failure mode I'm looking for, because I hate missing real bugs more than I hate processing failures that would have been fine except "that one little thing". But there are arguments for both.

TallJimbo · 2026-09-09T14:38:33Z

+        )
+        # Get the fallback PSF if available
+        # Even non-cell coadds could implement something similar
+        # Other exposures like single visits would be more work


I'm curious what you need a fallback PSF for. I think we've got a PSF anywhere we've got data.

I added a longer comment about this, but slightly more briefly:

For cells, the fallback PSF is only used to compute the size of the box for injection, which could overlap with other cells with valid PSFs. For example, DP2 ended up with cells where all of the visits were rejected because of a saturated star masking out >50% of the pixels, but we could still try to inject an object with its centroid in the corner of one of these cells if the neighbouring cells are fine.

This might be worth making a configurable option, along with the existing fallback to get the PSF at the nearest edge of the bbox for centroids outside the patch bounds.

One argument in favour of trying to inject wherever possible for coadds is that the injection runs for each band are independent, and we don't want to give up on one band prematurely if the injection is going to be successful in another.

leeskelvin · 2026-09-10T17:56:12Z

    pixel_scale = wcs.getPixelScale(bbox.getCenter()).asArcseconds()

+    if is_cell:
+        all_bounds = []


Setting this up with a proper type hint will help with a downstream linting error for:

conv = galsim.Convolve(object, bounds_info[3] / aperture_correction)

Something like:

all_bounds: list[ tuple[galsim.BoundsI, tuple[CellIJ, float, float, galsim.InterpolatedImage] | None] ] = []

To fix it though you'll need to add an assert just before you call bounds_info[3].

Longer-term, we should probably replace this with a named tuple or dict or something. Selecting random elements from a list downstream can be fraught with danger...

I'm going to take an easier solution and just drop the other elements in the tuple since I'm not using them anymore.

Well I immediately needed to add more items back in, so I guess it's a Pydantic dataclass now.

leeskelvin · 2026-09-10T17:56:43Z

+            if object_common_bounds.area() > 0:
+                if is_cell:
+                    # Use the per-cell PSF
+                    conv = galsim.Convolve(object, bounds_info[3] / aperture_correction)


Adding an assert here removes a linting error:

assert bounds_info is not None

Erm, what linting error? You mean from VSCode?

A Pylance error:

Object of type "None" is not subscriptable

I think because it knows nothing about the form of all_bounds, but it does know that the second element can be set to None elsewhere, so it incorrectly infers the type here.

I've changed this bit but if Pylance is still complaining, I'm not inclined to do anything about it if it's not running in pre-commit or GHA.

On that note, I had to update the Black version but we really should just drop it entirely in favour of ruff only. I can do that on another ticket since I had to modernize pre-commit/GH workflows for multiprofit recently.

leeskelvin · 2026-09-10T18:00:03Z

-                psf_array = psf.computeKernelImage(contained_point).array
-            except InvalidParameterError:
+        # Get the PSF at the centroid of the object
+        galsim_psf_centroid, aperture_correction, psf_error_args = _get_galsim_psf(


I'm not sure this still holds true in cell-space, where the aperture correction can also vary per cell if I understand correctly? The aperture correction calculated is global, determined at psf.getAveragePosition().

Well, I can apply the correction per-cell, but why was it using getAveragePosition in the first place?

As for why it was using the average position before, I don't know. Most likely something lost in the noise during development.

leeskelvin · 2026-09-10T19:34:47Z

-        flux0 = np.sum(self.exposure.image.array)
+        self._test_inject_galsim_objects_into_exposure(self.exposure, False)
+
+    def _test_inject_galsim_objects_into_exposure(self, exposure, is_cell: bool = True):


is_cell doesn't appear to be used in this method?

It also doesn't look like many of the cell-specific logic is being tested in this unit test. For example, you've set things up such that CellIJ(1, 1) is missing, but nothing here verifies that no sources were incorrectly skipped due to the missing cell. E.g., via a check of psf_compute_errors perhaps?

There's also no check of the mask conversion logic that's specific to cells, which we may also want to test.

There's a bit of an issue here - with non-cell coadds, if injection fails it leaves draw_size as zero. With cells, however, it's possible to successfully draw in some cells but not others. It's trivial to keep track of the number of pixels that were actually drawn but this is <= draw_size as currently defined. Should we only return the former, or both?

If we only return the original, intended draw_size then it's more difficult to test that the correct thing happened (drawing into zero pixels). I think it's less useful to know how many pixels GalSim wanted to inject into vs how many actually were, but I can see someone wanting both.

The existing tests are rather minimal too. We should be checking that INJECTED_CORE was set in the right places, that the number of INJECTED pixels and the flux within them is sensible, etc.

draw_size is intended to be the GalSim requested box size along one axis. For info on how many pixels we actually touched, we can look at our mask planes.

We don't have that per-object, though. I suppose one can compare draw_size^2 and the bounds in either case to see how much area was clipped/outside of the patch bbox, but with cells there's nothing reporting if an object was only injected into one cell vs the entire bounds.

leeskelvin · 2026-09-10T19:36:41Z

@@ -150,7 +170,7 @@ def test_inject_galsim_objects_into_exposure(self):
        pc = self.exposure.getPhotoCalib()


You caught the self.exposure -> exposure bug below already (thanks!) - it looks like there's another one here just above.

This is not a bug - cell coadds don't have a getPhotoCalib. But this should probably be moved to setUp since it only depends on self.exposure and self.pc.

leeskelvin · 2026-09-10T19:54:30Z

    add_noise: bool = True,
    noise_seed: int = 0,
    bad_mask_names: list[str] | None = None,
+    injection_core_size: float = 3.0,


This needs to be an int.

It's kind of misleading, though. What it actually does is make a galsim Bounds that's a single pixel and then expand it by injection_core_size//2. So passing in 2 <= injection_core_size < 4 has the exact same result and 4 <= injection_core_size < 6 gives a 5x5 box, etc.

Right - it must be an odd number such that the centroid falls within a pixel. I believe this is documented on pipelines.lsst.io somewhere, but if not, it probably should be.

leeskelvin · 2026-09-28T21:33:34Z

            if logger:
-                logger.debug("No area overlap for object at %s; flagging and skipping.", sky_coords)
+                logger.debug(
+                    "GalSimFFTSizeError%s raised for object at index=%d and coords %s;"


I think the %s after GalSimFFTSizeError is a typo here.

This is mainly to improve the readibility of a very long function that will need more specialization for cell coadds.

This shortens the very long inject_galsim_objects_into_exposure method in advance of it growing again to support cell coadds, albeit at the expense of adding a private method with many (keyword) arguments.

taranu · 2026-10-02T20:10:23Z

+    # While None might perhaps have been a better choice here, changing the
+    # return type is unnecessary given that galsim.Shear can be initialized
+    # with no args and correctly returns a trivial shear.
+    if all(np.ma.is_masked(x) for x in shear_data.values()):


FYI @leeskelvin I added this because I think it's correct behaviour, although it's debatable for non-DeltaFunction profiles.

Previously I think it was falling back to the underlying value of the masked shear columns for a DeltaFunction and getting a trivial shear and applying it. I don't think that's particularly reliable and it may be why some users were having issues (see here for example) though I was never able to replicate them.

This adds galaxy injection and more thorough testing for missing cells.

taranu force-pushed the tickets/DM-53757 branch 2 times, most recently from 6787211 to 4cb6622 Compare September 9, 2026 04:14

TallJimbo approved these changes Sep 9, 2026

View reviewed changes

taranu force-pushed the tickets/DM-53757 branch from 4cb6622 to 2234138 Compare September 9, 2026 23:03

leeskelvin reviewed Sep 10, 2026

View reviewed changes

taranu force-pushed the tickets/DM-53757 branch from 2234138 to 134b220 Compare September 21, 2026 16:25

leeskelvin reviewed Sep 28, 2026

View reviewed changes

taranu added 12 commits October 1, 2026 15:59

Update black/python version in pre-commit

6b17d20

Move PSF image computation to _get_galsim_psf

da0d42c

This is mainly to improve the readibility of a very long function that will need more specialization for cell coadds.

Move object injection into separate method

f7c5e72

This shortens the very long inject_galsim_objects_into_exposure method in advance of it growing again to support cell coadds, albeit at the expense of adding a private method with many (keyword) arguments.

Implement injecting into cell coadds

40171c9

Add make_test_cell_coadd to convert legacy exposures

085e1f7

Add unit test for cell coadd injection

bbfd485

Normalize PSF before applying aperture correction

03c62a3

Make injection_core_size a kwarg

04adcaa

Fix remaining cell injection problems

a3058e3

Return empty shear dict if values are all masekd

cca80bb

Implement cell injection using same PSF for identical-input cells

8457271

Add galaxy to injection test

b2dbdaa

taranu force-pushed the tickets/DM-53757 branch from 134b220 to cd28ea6 Compare October 1, 2026 23:03

taranu commented Oct 2, 2026

View reviewed changes

taranu force-pushed the tickets/DM-53757 branch from cd28ea6 to 8fc999d Compare October 5, 2026 19:36

taranu added 4 commits October 5, 2026 16:07

Add CellCoadd I/O support to CoaddInjectTask

413196a

Inject Iterable of objects instead of Generator

9e033c2

Only add core mask in cell containing centroid

98999b8

Expand injection unit test

b24e552

This adds galaxy injection and more thorough testing for missing cells.

taranu force-pushed the tickets/DM-53757 branch from 8fc999d to b24e552 Compare October 6, 2026 00:04

		@@ -150,7 +170,7 @@ def test_inject_galsim_objects_into_exposure(self):
		pc = self.exposure.getPhotoCalib()

Conversation

taranu commented Sep 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taranu Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

taranu Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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!

Reviewers

taranu Sep 9, 2026 •

edited

Loading

taranu Sep 9, 2026 •

edited

Loading