# Change estimateBackground to not deep copy the exposure and make it a task

XMLWordPrintable

## Details

• Type: RFC
• Status: Implemented
• Resolution: Done
• Component/s:
• Labels:
None

## Description

estimateBackground deep copies the exposure if subtract=True and returns the copy. This seems a waste of memory and time with no real benefit for the user. In my opinion it should simply modify the exposure in place. In addition, it returns the exposure or None if background not subtracted; this is clumsy and seems unnecessary if it modifies the exposure in place. Also it has a ConfigClass attribute so I suggest that it be made a task.

My full proposal is:

• Modify the exposure in place if subtracting the background
• Accept the current background model, make a shallow copy and return the new background. That way if background is not to be subtracted the input background model is still valid for the exposure.

## Activity

Hide
Jim Bosch added a comment -

Agree with the proposal, but we should make sure in the implementation to do a careful audit of calling code - I will be surprised if we don't have calling code that relies on the result being a deep-copy in a way that isn't unit-tested.

Show
Jim Bosch added a comment - Agree with the proposal, but we should make sure in the implementation to do a careful audit of calling code - I will be surprised if we don't have calling code that relies on the result being a deep-copy in a way that isn't unit-tested.
Hide
Russell Owen added a comment -

Jim Bosch I agree that we'll have to be careful about replacement. Fortunately estimateBackground is only used in three places, and the deep copying is not relied upon. Those places are:

• CharacterizeImageTask, which would benefit from this change
• an example, which will work either way

The code in RepairTask is interesting because it explicitly deep-copies the exposure before calling estimateBackground. This was redundant, and I think demonstrates the value of changing this confusing API, but it will not be redundant if we adopt this RFC:

 exposure = exposure.Factory(exposure, True) modelBg, exposure = measAlg.estimateBackground(exposure, self.config.cosmicray.background,  subtract=True) 

Note that it would be more efficient to deep copy the image, not the whole exposure.

Show
Russell Owen added a comment - Jim Bosch I agree that we'll have to be careful about replacement. Fortunately estimateBackground is only used in three places, and the deep copying is not relied upon. Those places are: CharacterizeImageTask, which would benefit from this change RepairTask (see below) an example, which will work either way The code in RepairTask is interesting because it explicitly deep-copies the exposure before calling estimateBackground . This was redundant, and I think demonstrates the value of changing this confusing API, but it will not be redundant if we adopt this RFC: exposure = exposure.Factory(exposure, True) modelBg, exposure = measAlg.estimateBackground(exposure, self.config.cosmicray.background, subtract=True) Note that it would be more efficient to deep copy the image, not the whole exposure.
Hide
Russell Owen added a comment - - edited

My original proposal had a serious weakness: modifying the exposure in place but not modifying the background model is a recipe for confusion; users are likely to end up with the wrong background model. Furthermore, no existing code simply estimates the background without also subtracting it.

I now propose the following for estimateBackground

• Always subtract the background
• Modify both the exposure and the background model in place, returning nothing

If in the future a user wants to merely fit a new background model without changing anything then the user can copy the exposure and background model before calling the function.

I will extend the open time so this new proposal can be digested.

Show
Russell Owen added a comment - - edited My original proposal had a serious weakness: modifying the exposure in place but not modifying the background model is a recipe for confusion; users are likely to end up with the wrong background model. Furthermore, no existing code simply estimates the background without also subtracting it. I now propose the following for estimateBackground Always subtract the background Modify both the exposure and the background model in place, returning nothing If in the future a user wants to merely fit a new background model without changing anything then the user can copy the exposure and background model before calling the function. I will extend the open time so this new proposal can be digested.
Hide
Jim Bosch added a comment -

This proposal sounds entirely reasonable, but I confess I'm not familiar enough with how the algorithm is typically invoked to have an expert opinion.

Show
Jim Bosch added a comment - This proposal sounds entirely reasonable, but I confess I'm not familiar enough with how the algorithm is typically invoked to have an expert opinion.
Hide
Russell Owen added a comment -

• Always subtract the background
• Modify the exposure in place
• Return the background model (this cannot be updated in place because the code does not take a background model)
• Make this a task (since it has a config attached)
Show
Russell Owen added a comment - Adopted as follows: Always subtract the background Modify the exposure in place Return the background model (this cannot be updated in place because the code does not take a background model) Make this a task (since it has a config attached)
Hide
Russell Owen added a comment -

For the record: this function is only called in three places:

 Users/rowen/UW/LSST/lsstsw/build/pipe_tasks/examples/detectAndMeasureExample.py:  85   86 # subtract an initial estimate of background level  87: estBg, exposure = estimateBackground(  88 exposure = exposure,  89 backgroundConfig = backgroundConfig,   /Users/rowen/UW/LSST/lsstsw/build/pipe_tasks/python/lsst/pipe/tasks/characterizeImage.py:  348   349 # subtract an initial estimate of background level  350: estBg = estimateBackground(  351 exposure = exposure,  352 backgroundConfig = self.config.background,   /Users/rowen/UW/LSST/lsstsw/build/pipe_tasks/python/lsst/pipe/tasks/repair.py:  246 else:  247 exposure = exposure.Factory(exposure, True)  248: modelBg, exposure = measAlg.estimateBackground(exposure, self.config.cosmicray.background,  249 subtract=True)  250 medianBg = 0.0 

Show
Russell Owen added a comment - For the record: this function is only called in three places: Users/rowen/UW/LSST/lsstsw/build/pipe_tasks/examples/detectAndMeasureExample.py: 85 86 # subtract an initial estimate of background level 87: estBg, exposure = estimateBackground( 88 exposure = exposure, 89 backgroundConfig = backgroundConfig,   /Users/rowen/UW/LSST/lsstsw/build/pipe_tasks/python/lsst/pipe/tasks/characterizeImage.py: 348 349 # subtract an initial estimate of background level 350: estBg = estimateBackground( 351 exposure = exposure, 352 backgroundConfig = self.config.background,   /Users/rowen/UW/LSST/lsstsw/build/pipe_tasks/python/lsst/pipe/tasks/repair.py: 246 else: 247 exposure = exposure.Factory(exposure, True) 248: modelBg, exposure = measAlg.estimateBackground(exposure, self.config.cosmicray.background, 249 subtract=True) 250 medianBg = 0.0
Hide
Russell Owen added a comment -

estimateBackground calls getBackground to actually do the background estimation, and both use the same config. Thus I plan to make getBackground a method of the new task and rename it to fitBackground. It will have the same API as getBackground except that it does not take a config as an argument.

Show
Russell Owen added a comment - estimateBackground calls getBackground to actually do the background estimation, and both use the same config. Thus I plan to make getBackground a method of the new task and rename it to fitBackground . It will have the same API as getBackground except that it does not take a config as an argument.

## People

• Assignee:
Russell Owen
Reporter:
Russell Owen
Watchers:
Jim Bosch, Russell Owen