DM-46693: Add ability to re-apply bgModel1 in SkyCorrectionTask #989

aemerywatkins · 2024-10-09T15:38:50Z

The current SkyCorrectionTask has three principal steps:

bgModel1 - a large-scale sky estimation and subtraction
sky - a subtraction of the sky frame
bgModel2 - a small-scale sky estimation and subtraction

If one has doSky = True, then bgModel1 is implied. However, during testing of DM-44889, it became apparent that the need for a doSky = True -only dataset output was required. This necessitates that bgModel1 is generated, used, and then undone.

This ticket adds that functionality into the current SkyCorrectionTask.

Note the prior related removal ticket of doBgModel1, DM-37429, which may be relevant/of use here.

fred3m

Just a few minor comments, otherwise LGTM.

fred3m · 2024-10-10T15:56:16Z

python/lsst/pipe/tasks/skyCorrection.py

+from lsst.pipe.base import (PipelineTask, PipelineTaskConfig,
+                            PipelineTaskConnections, Struct)
+from lsst.pipe.tasks.background import (FocalPlaneBackground,
+                                        FocalPlaneBackgroundConfig,
+                                        MaskObjectsTask, SkyMeasurementTask)
+from lsst.pipe.tasks.visualizeVisit import (VisualizeMosaicExpConfig,
+                                            VisualizeMosaicExpTask)


Is there a reason that this formatting changed? In general we prefer not to make changes that are merely stylistic that depend on the coders style preference, and the older version is the preferred black formatting, so I would undo this change.

I agree, this formatting change should probably be reverted. When I run this file through my black/isort formatters, it also reverts it to its original format as well.

fred3m · 2024-10-10T16:12:30Z

python/lsst/pipe/tasks/skyCorrection.py

+    doBgModel1 = Field(
+        dtype=bool,
+        default=True,
+        doc="If False, adds back initial background model after sky",
+    )


I would change the name of this config parameter. I see the symmetry in making it similar to doBgModel2 but in that case the config option chooses whether or not to do the final cleanup and background subtraction. Here, it looks like the initial background subtraction is done in order to do the sky subtraction, then it is added back in if doBgModel1 == False. So I would recommend calling this restoreOriginalBackground or something similar that doesn't make it sound as if the model 1 background subtraction is skipped completely.

How about restoreBgModel1? To keep the symmetry but clarify the usage. And then the default value will swap to False.

Maybe I misunderstand the code, but it isn't restoring bgModel1, right? I thought that it's restoring the background before bgModel1 was made and subtracted. Maybe undoBgModel1 or removeBgModel1 or revertBgModel1?

The order of operations facilitated here is:

subtract bgModel1

subtract sky

remove bgModel1 from the background list

subtract bgModel2

At the end of this, the skyCorr output background list should consist of:

the inverted original detector-level background elements (4 of them)

no bgModel1 (this has been used to fit the sky frame, and then removed from the bg list)

sky frame

bgModel2

With that in mind, I do agree that naming this config doBgModel1 makes it seem like a similar operation to doBgModel2, but it does act differently. I wasn't sure which alternative to endorse; I realize that in describing the order of operations and dataset outputs above I twice referred to this as a removal. However, out of context, "remove BG Model 1" sounds like you're subtracting the model, which is the opposite of what we're doing here.

So with that said, I think undoBgModel1 is a nice compromise. I think we would need to make the usage of this config clear in the doc string though.

leeskelvin · 2024-10-11T13:22:17Z

python/lsst/pipe/tasks/skyCorrection.py

+
+        Parameters
+        ----------
+        calExps : `list` [`lsst.afw.image.exposure.ExposureF`]


This can just be:

calExps : `list` [`lsst.afw.image.ExposureF`]

(it probably needs to be changed in the doc-strings across this file at the same time too)

leeskelvin · 2024-10-11T13:22:38Z

python/lsst/pipe/tasks/skyCorrection.py

+        ----------
+        calExps : `list` [`lsst.afw.image.exposure.ExposureF`]
+            Calibrated exposures to be background subtracted.
+        calBkgs : `list` [`lsst.afw.math._backgroundList.BackgroundList`]


This can also just be:

calBkgs : `list` [`lsst.afw.math.BackgroundList`]

(and in the returns below)

leeskelvin · 2024-10-11T13:29:30Z

python/lsst/pipe/tasks/skyCorrection.py

+        Returns
+        -------
+        calExps : `list` [`lsst.afw.image.maskedImage.MaskedImageF`]
+            Background subtracted exposures for creating a focal plane image.
+        calBkgs : `list` [`lsst.afw.math._backgroundList.BackgroundList`]
+            Updated background lists with a visit-level model removed.


There's a return section here, but this function doesn't seem to return anything? Instead, it seems to edit in-place?

leeskelvin · 2024-10-11T14:02:41Z

python/lsst/pipe/tasks/skyCorrection.py

+            calBkg._backgrounds.pop(-2)
+
+            self.log.info(
+                "Detector %d: FFP background restored",


I think this log message could do with being more verbose, e.g.:

"Detector %d: Initial background model prior to sky frame subtraction (bgModel1) has been restored",

This does however now clash with usage of "initial background" on lines 377 and 396, which refer to the "original background" at the calexp level. I would favor renaming these two instances to "original background", and preserve initial background as a reference to bgModel1.

leeskelvin · 2024-10-11T14:06:07Z

python/lsst/pipe/tasks/skyCorrection.py

@@ -387,6 +400,37 @@ def _restoreBackgroundRefineMask(self, calExps, calBkgs):
            skyCorrBases.append(skyCorrBase)
        return calExps, skyCorrBases

+    def _restoreVisitBackground(self, calExps, calBkgs):


I would suggest renaming this to _restoreInitialBackground.

In tandem with this, the current method _restoreBackgroundRefineMask should probably also be renamed to _restoreOriginalBackgroundRefineMask, to avoid any potential confusion.

PS - if we're changing the associated config to undoBgModel1, it probably also makes sense to rename this method _undoInitialBackground as well.

leeskelvin · 2024-10-11T14:07:38Z

python/lsst/pipe/tasks/skyCorrection.py

+
+        Returns
+        -------
+        calExps : `list` [`lsst.afw.image.maskedImage.MaskedImageF`]


This worried me originally, as the input calExps are ExposureF, but the output is a MaskedImageF? However, I think this returns section is going away in any case (see other related comment). You can probably replace this section with a Notes stating that the inputs are modified in-place.

fred3m

Looks like a couple of linting changes required to pass the tests. Other than that this LGTM.

aemerywatkins force-pushed the tickets/DM-46693 branch 2 times, most recently from 1fef450 to a4066ff Compare October 10, 2024 15:04

aemerywatkins marked this pull request as ready for review October 10, 2024 15:06

aemerywatkins requested a review from fred3m October 10, 2024 15:36

fred3m reviewed Oct 10, 2024

View reviewed changes

leeskelvin reviewed Oct 11, 2024

View reviewed changes

leeskelvin force-pushed the tickets/DM-46693 branch from bfeda7e to ad3505d Compare October 18, 2024 19:37

leeskelvin force-pushed the tickets/DM-46693 branch 3 times, most recently from 739b097 to 65fed08 Compare November 3, 2024 14:01

leeskelvin force-pushed the tickets/DM-46693 branch 5 times, most recently from 9fda939 to 5b99ff2 Compare December 17, 2024 18:29

fred3m approved these changes Dec 17, 2024

View reviewed changes

leeskelvin force-pushed the tickets/DM-46693 branch from 5b99ff2 to 5f44b71 Compare December 17, 2024 19:01

Add ability to undo bgModel1 in SkyCorrectionTask

c94974e

leeskelvin force-pushed the tickets/DM-46693 branch from 5f44b71 to d09b471 Compare December 17, 2024 19:17

Add unit tests for SkyCorrectionTask

db2c9e0

leeskelvin force-pushed the tickets/DM-46693 branch from d09b471 to db2c9e0 Compare December 17, 2024 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-46693: Add ability to re-apply bgModel1 in SkyCorrectionTask #989

DM-46693: Add ability to re-apply bgModel1 in SkyCorrectionTask #989

aemerywatkins commented Oct 9, 2024 •

edited by jira bot

Loading

fred3m left a comment

fred3m Oct 10, 2024

leeskelvin Oct 11, 2024

fred3m Oct 10, 2024

aemerywatkins Oct 11, 2024

fred3m Oct 11, 2024

leeskelvin Oct 11, 2024

leeskelvin Oct 11, 2024

leeskelvin Oct 11, 2024 •

edited

Loading

leeskelvin Oct 11, 2024

leeskelvin Oct 11, 2024

leeskelvin Oct 11, 2024

leeskelvin Oct 11, 2024

leeskelvin Oct 11, 2024

leeskelvin Oct 11, 2024

leeskelvin Oct 11, 2024

fred3m left a comment

DM-46693: Add ability to re-apply bgModel1 in SkyCorrectionTask #989

Are you sure you want to change the base?

DM-46693: Add ability to re-apply bgModel1 in SkyCorrectionTask #989

Conversation

aemerywatkins commented Oct 9, 2024 • edited by jira bot Loading

fred3m 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

leeskelvin Oct 11, 2024 • edited Loading

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

fred3m left a comment

Choose a reason for hiding this comment

aemerywatkins commented Oct 9, 2024 •

edited by jira bot

Loading

leeskelvin Oct 11, 2024 •

edited

Loading