Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-5323

estimateBackground should not make a deep copy of the exposure

    Details

    • Type: Improvement
    • Status: Done
    • Priority: Major
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_algorithms
    • Labels:
      None
    • Templates:
    • Story Points:
      4
    • Sprint:
      Alert Production X16 - 5
    • Team:
      Alert Production

      Description

      Implement RFC-155: change estimateBackground as follows:

      • Always subtract the background
      • Modify the exposure in place
      • Replace estimateBackground with the run method of a new task SubtractBackgroundTask
      • Replace getBackground (which fits a background) with SubtractBackgroundTask.fitBackground

        Issue Links

          Activity

          Hide
          rowen Russell Owen added a comment - - edited

          Most of the work is in meas_algorithms, but it affected some code in ip_diffim, pipe_drivers and pipe_tasks. It also required a trivial change to a comment in meas_base.

          Show
          rowen Russell Owen added a comment - - edited Most of the work is in meas_algorithms , but it affected some code in ip_diffim , pipe_drivers and pipe_tasks . It also required a trivial change to a comment in meas_base .
          Hide
          price Paul Price added a comment -

          Review completed on GitHub PRs. Besides some little things, the only major comment I have is that the SubtractBackgroundTask.run method should be made as compact as possible by delegating work to other methods.

          Show
          price Paul Price added a comment - Review completed on GitHub PRs. Besides some little things, the only major comment I have is that the SubtractBackgroundTask.run method should be made as compact as possible by delegating work to other methods.
          Hide
          rowen Russell Owen added a comment -

          Thank you for the comprehensive review. It was very helpful.

          I agree that computing statistics on the background is a poor idea for this task, but it appears to be necessary for now, so I carried it over from existing code and filed RFC-180. I also agree that the background subtraction in `ip_diffim` could be cleaned up a bit, but I feel that would be best left for a more comprehensive refactoring. I was wrong about the tight coupling between the task's config and `makeBackground`'s control object, so I updated the documentation accordingly.

          Aside from that, I believe I implemented all of your suggested changes. I also modernized the display code in `SubtractBackgroundTask` and rebased from master. Once CI passes I will merge.

          Show
          rowen Russell Owen added a comment - Thank you for the comprehensive review. It was very helpful. I agree that computing statistics on the background is a poor idea for this task, but it appears to be necessary for now, so I carried it over from existing code and filed RFC-180 . I also agree that the background subtraction in `ip_diffim` could be cleaned up a bit, but I feel that would be best left for a more comprehensive refactoring. I was wrong about the tight coupling between the task's config and `makeBackground`'s control object, so I updated the documentation accordingly. Aside from that, I believe I implemented all of your suggested changes. I also modernized the display code in `SubtractBackgroundTask` and rebased from master. Once CI passes I will merge.
          Hide
          rowen Russell Owen added a comment -
          Show
          rowen Russell Owen added a comment - The changes are described here: https://community.lsst.org/t/changes-in-how-background-subtraction-is-done/756

            People

            • Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Paul Price
              Watchers:
              Paul Price, Russell Owen
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile