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

[aemerywatkins]

The current SkyCorrectionTask has three principal steps:

1. bgModel1 - a large-scale sky estimation and subtraction

2. sky - a subtraction of the sky frame

3. 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]

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.

[leeskelvin]

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]

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.

[aemerywatkins]

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

[fred3m]

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?

[leeskelvin]

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.

[leeskelvin]

This can just be:

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

[leeskelvin]

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

[leeskelvin]

This can also just be:

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

[leeskelvin]

(and in the returns below)

[leeskelvin]

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

[leeskelvin]

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]

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.

[leeskelvin]

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

[leeskelvin]

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.