DM-55521: update coadd processing tasks to work with lsst.images by TallJimbo · Pull Request #1334 · lsst/pipe_tasks

TallJimbo · 2026-07-20T19:22:27Z

No description provided.

taranu

Looks good; see comments and questions.

I assume most of this is going to be tested through CI pipelines but can anything be covered by basic unit tests here, e.g. _ensureMaskPlanes?

taranu · 2026-08-06T22:15:42Z

        doc="Task to deblend an images in multiple bands"
    )
    idGenerator = SkyMapIdGeneratorConfig.make_field()
+    imageType = ChoiceField(


Hmm, are you anticipating keeping this around and adding to allowed at some point, even after legacy is deprecated? Or did you mainly want better documentation via the allowed dict than you'd get from a boolean Field?

I'm not planning to keep it around forever, but on this ticket I really wished that useCellCoadd had been a ChoiceField instead a boolean, because then I would have been able to use that instead of inventing another config field and leaving useCellCoadd sort of dangling in a lot of cases. So this is probably fighting the last war, but I figured I'd leave room for a plot twist in the migration just in case.

taranu · 2026-08-07T02:16:20Z

+                `lsst.images.cells.CellCoadd`]
+            Coadds to deblend.
+        bands : `list` [`str`]
+            Names or the bands for ``coadds`` (zip-iteration compatible).


Are there lists that aren't zip-iteration compatible, or do you just mean that the bands have to be in the same order as the coadds?

Yes, I mean it should have corresponding elements in the same order.

taranu · 2026-08-07T02:21:56Z

    "CoaddPsfFitSubConfig", "CoaddPsfFitSubTask", "CoaddPsfFitTask",
 ]

+


Accidental, or did you think a new import was going to be needed?

Accidental; fixed.

taranu · 2026-08-10T18:26:37Z

        doc="Task to fit PSF models for a single coadd",
    )
    idGenerator = SkyMapIdGeneratorConfig.make_field()
+    image_type = pexConfig.ChoiceField(


Would it make sense to define this once in a module somewhere, or is it safer to duplicate?

The docstrings pretty much always differ, even if only slightly sometimes.

taranu · 2026-08-10T18:29:08Z

+        "output images are lsst.images.cells.CellCoadd.",
+        dtype=str,
+        default=(
+            "Background subtracted from the image when generating the Object catalog. "


Would it make sense to put this string in a separate module, if not the whole Field?

Yeah, I'm not super comfortable having all of this full-pipeline context here, either. But I didn't identify any alternatives that I liked a lot better.

Some options:

Just move it to a module-level constant in this same module, so it's at least importable on its own and a little less buried.

Make the default empty or much more generic, and override it in drp_pipe.

If you like one of those better, I'm happy to go with it.

I slightly prefer the first option.

taranu · 2026-08-11T18:14:40Z

            backgrounds.append(emptyBg)
-
+        if cell_coadd is not None:
+            cell_coadd.image.array[...] = exposure.image.array


Huh... is there any functional difference between [...] and [:] and .flat?

In this limit, if you do .flat on both of them, I think it'll amount to the same. But [...] on a 2-d array like this actually resolves to [:, :] (i.e. still 2-d), so it would definitely behave differently if broadcasting was in play, and there may be other differences as well.

taranu · 2026-08-11T18:27:52Z

        results.outputSources = sources
        return results
+
+    def _ensureMaskPlanes(self):


Is this for (current or future) backwards compatibility if running on coadds without newer masks like SPIKE? It's worth a comment regardless.

Added this docstring:

"""Ensure the global mask dictionary has all of the mask planes needed for PixelFlags algorithms. When mask planes are added, this essentially guarantees that the corresponding PixelFlags columns will be wholly False, and usually we'd prefer to remove them from the configuration. But those config changes imply a schema changes, and that's not always viable (e.g. on a release branch). """

Adding error information to metadata doesn't do much if we write it to disk first.

Unlike other changes so far on this ticket, this one only modifies 'runQuantum'/connections and leaves 'run' only supporting Exposure as input. Fixing that would make everything a lot messier unless we do a lot of work, like making cell-coadd aperture corrections a true BoundedField and/or dropping MultipleCellCoadd as a supported connection type (RFC-1193).

This only touches the connection/runQuantum level; run still only works on lsst.afw.image.Exposure.

In the long term, it would be better to un-configure those PixelFlags columns, but that changes the schema and hence isn't viable for something we want to backport to v30.

TallJimbo · 2026-08-12T20:45:50Z

I assume most of this is going to be tested through CI pipelines but can anything be covered by basic unit tests here, e.g. _ensureMaskPlanes?

There's just so little test scaffolding for these tasks available that it doesn't seem worthwhile to add all of that just for a little change like this. Of course that's self-perpetuating, but I can at least confirm that ci_lsstcam definitely fails without that _ensureMaskPlanes fix.

TallJimbo force-pushed the tickets/DM-55521 branch 12 times, most recently from 2dbea16 to 7b592c6 Compare July 23, 2026 22:37

taranu approved these changes Aug 11, 2026

View reviewed changes

TallJimbo added 8 commits August 12, 2026 11:55

Fix bad docs on DetectCoaddSourceTask return value.

156383a

Move error annotation before butler.put.

f2a1ddb

Adding error information to metadata doesn't do much if we write it to disk first.

Update DetectCoaddSourcesTask to work with lsst.images.

6150aac

Update DeblendCoaddSourcesTask to work with lsst.images.

5994152

Update multiprofit base tasks to work with lsst.images.

8a2832e

This only touches the connection/runQuantum level; run still only works on lsst.afw.image.Exposure.

Warn instead of failing when a coadd lacks PixelFlags planes.

627264b

In the long term, it would be better to un-configure those PixelFlags columns, but that changes the schema and hence isn't viable for something we want to backport to v30.

Stop reading coadd PhotoCalibs in healsparse map-making.

bfb8637

TallJimbo force-pushed the tickets/DM-55521 branch from e3e75a8 to bfb8637 Compare August 12, 2026 15:56

Make the deep_coadd background docstring a module-level constant.

82b9d23

TallJimbo force-pushed the tickets/DM-55521 branch from 0333799 to 82b9d23 Compare August 12, 2026 20:53

TallJimbo merged commit 77d882f into main Aug 13, 2026
4 checks passed

TallJimbo deleted the tickets/DM-55521 branch August 13, 2026 14:18

		"CoaddPsfFitSubConfig", "CoaddPsfFitSubTask", "CoaddPsfFitTask",
		]

Uh oh!

Conversation

TallJimbo commented Jul 20, 2026

Uh oh!

taranu 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!

Choose a reason for hiding this comment

Uh oh!

TallJimbo commented Aug 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants