Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-155

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

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • 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.
      • Make estimateBackground a task

        Attachments

          Issue Links

            Activity

            Hide
            jbosch 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
            jbosch 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
            rowen 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.

            Show
            rowen 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
            rowen 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
            rowen 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
            jbosch 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
            jbosch 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
            rowen 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)
            Show
            rowen 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
            rowen 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
            rowen 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
            rowen 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
            rowen 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:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Watchers:
                Jim Bosch, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel