Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4066ff
Add ability to re-apply bgModel1 in SkyCorrectionTask
aemerywatkins Oct 9, 2024
bfe1508
Refactor based on pull request review, AEW
aemerywatkins Oct 11, 2024
8d2c5cd
LSK edits
leeskelvin Oct 14, 2024
65fed08
Add unit tests for SkyCorrectionTask
leeskelvin Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 52 additions & 8 deletions python/lsst/pipe/tasks/skyCorrection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,13 @@
import lsst.pipe.base.connectionTypes as cT
import numpy as np
from lsst.pex.config import Config, ConfigField, ConfigurableField, Field
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
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def _skyFrameLookup(datasetType, registry, quantumDataId, collections):
Expand Down Expand Up @@ -178,6 +177,11 @@ class SkyCorrectionConfig(PipelineTaskConfig, pipelineConnections=SkyCorrectionC
dtype=FocalPlaneBackgroundConfig,
doc="Initial background model, prior to sky frame subtraction",
)
doBgModel1 = Field(
dtype=bool,
default=True,
doc="If False, adds back initial background model after sky",
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of operations facilitated here is:

  1. subtract bgModel1
  2. subtract sky
  3. remove bgModel1 from the background list
  4. subtract bgModel2

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

  1. the inverted original detector-level background elements (4 of them)
  2. no bgModel1 (this has been used to fit the sky frame, and then removed from the bg list)
  3. sky frame
  4. 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.

sky = ConfigurableField(
target=SkyMeasurementTask,
doc="Sky measurement",
Expand Down Expand Up @@ -210,6 +214,11 @@ def setDefaults(self):
self.bgModel2.ySize = 256
self.bgModel2.smoothScale = 1.0

def validate(self):
super().validate()
if not self.doBgModel1 and not self.doSky and not self.doBgModel2:
raise ValueError("Task requires at least one of doBgModel1, doSky or doBgModel2 to be True.")


class SkyCorrectionTask(PipelineTask):
"""Perform a full focal plane sky correction."""
Expand Down Expand Up @@ -310,6 +319,10 @@ def run(self, calExps, calBkgs, skyFrames, camera):
if self.config.doSky:
self._subtractSkyFrame(calExps, skyFrames, calBkgs)

# Adds full-fp bg back onto exposures, removes it from list
if not self.config.doBgModel1:
_ = self._restoreVisitBackground(calExps, calBkgs)

# Bin exposures, generate full-fp bg, map to CCDs and subtract in-place
if self.config.doBgModel2:
_ = self._subtractVisitBackground(calExps, calBkgs, camera, self.config.bgModel2)
Expand Down Expand Up @@ -387,6 +400,37 @@ def _restoreBackgroundRefineMask(self, calExps, calBkgs):
skyCorrBases.append(skyCorrBase)
return calExps, skyCorrBases

def _restoreVisitBackground(self, calExps, calBkgs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""Restores the full focal-plane background to a visit.
Runs after _subtractSkyFrame() if doBgModel1=False.

Parameters
----------
calExps : `list` [`lsst.afw.image.exposure.ExposureF`]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be:

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

Copy link
Contributor

@leeskelvin leeskelvin Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Calibrated exposures to be background subtracted.
calBkgs : `list` [`lsst.afw.math._backgroundList.BackgroundList`]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also just be:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and in the returns below)

Background lists associated with the input calibrated exposures.

Returns
-------
calExps : `list` [`lsst.afw.image.maskedImage.MaskedImageF`]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
for calExp, calBkg in zip(calExps, calBkgs):
image = calExp.getMaskedImage()

# Restore full focal-plane background in calexp; remove from BGList
skyCorrBase = calBkg[-2][0].getImageF()
image += skyCorrBase
calBkg._backgrounds.pop(-2)

self.log.info(
"Detector %d: FFP background restored",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

calExp.getDetector().getId(),
)

def _subtractVisitBackground(self, calExps, calBkgs, camera, config):
"""Perform a full focal-plane background subtraction for a visit.

Expand Down
Loading