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

Fix order of operations when using temporary local backgrounds in detection

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_algorithms
    • Labels:
    • Story Points:
      3
    • Sprint:
      DRP S17-5
    • Team:
      Data Release Production

      Description

      Our current implementation of the temporary local background approach to avoiding spurious detections near bright objects simply subtracts a local background from the full image before performing any detection steps. That can result in missed isolated-object detections and incorrect Footprints for large objects.

      Instead, we should:
      1. Detect Footprints and Peaks.
      2. Subtract the local background.
      3. Detect Peaks within each Footprint again, and use the new set of Peaks instead of the old set if and only if there is at least one Peak in the new set.

      This really ought to be fixed before the HSC internal release or major HSC processing at NCSA.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            processCcd.py on one CCD, before this change:

            real	2m30.659s
            user	2m10.405s
            sys	0m25.486s
            

            and after:

            real	3m17.279s
            user	2m17.502s
            sys	0m26.649s
            

            Show
            jbosch Jim Bosch added a comment - processCcd.py on one CCD, before this change: real 2m30.659s user 2m10.405s sys 0m25.486s and after: real 3m17.279s user 2m17.502s sys 0m26.649s
            Hide
            price Paul Price added a comment -

            I've made some comments on the GitHub PRs, including suggestions for optimisation.

            Show
            price Paul Price added a comment - I've made some comments on the GitHub PRs, including suggestions for optimisation.
            Hide
            jbosch Jim Bosch added a comment -

            Performance regression fixed; the time to run detection on my test coadd patch has gone from 162s down 3.6s. I'm now doing the thresholding for the peaks only in the subimage covered by the footprint whose peaks we're trying to replace, which cuts down on both the comparison between footprints and the amount of image we have to threshold again (especially because I'm not trying to replace the peaks of footprints with only one peak).

            It looks like processCcd time spent on detection was always in the noise; my latest benchmark is:

            real	3m38.712s
            user	2m12.946s
            sys	0m25.709s
            

            Paul Price, if you're free to take another look today, please do, but I'll go ahead and merge later today if I don't hear from you as I believe I've addressed your biggest concern.

            Show
            jbosch Jim Bosch added a comment - Performance regression fixed; the time to run detection on my test coadd patch has gone from 162s down 3.6s. I'm now doing the thresholding for the peaks only in the subimage covered by the footprint whose peaks we're trying to replace, which cuts down on both the comparison between footprints and the amount of image we have to threshold again (especially because I'm not trying to replace the peaks of footprints with only one peak). It looks like processCcd time spent on detection was always in the noise; my latest benchmark is: real 3m38.712s user 2m12.946s sys 0m25.709s Paul Price , if you're free to take another look today, please do, but I'll go ahead and merge later today if I don't hear from you as I believe I've addressed your biggest concern.
            Hide
            price Paul Price added a comment -

            Had a quick look, and made some trivial comments on the GitHub PR.

            Show
            price Paul Price added a comment - Had a quick look, and made some trivial comments on the GitHub PR.
            Hide
            jbosch Jim Bosch added a comment -

            Merged to master. Thanks for the very helpful review, Paul Price (and Lauren MacArthur).

            Show
            jbosch Jim Bosch added a comment - Merged to master. Thanks for the very helpful review, Paul Price (and Lauren MacArthur ).

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                Bob Armstrong, John Swinbank, Paul Price
                Watchers:
                Bob Armstrong, Jim Bosch, John Swinbank, Paul Price, Robert Lupton
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel