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

Footprint dilation performance regression

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None

      Description

      In DM-1128 we implemented span-based dilation for footprints. A brief test on synthetic data indicated that this was a performance win over the previous version of the code.

      In May 2015, this code was merged to HSC and applied to significant quantities of real data for the first time. A major performance regression was identified:

      [May-9 00:26] Paul Price: processCcd is now crazy slow.
      [May-9 00:29] Paul Price: Profiling...
      [May-9 00:40] Paul Price: I'm thinking it's the Footprint grow code...
      [May-9 00:44] Paul Price: And the winner is…. Footprint construction:
      [May-9 00:44] Paul Price: 2 0.000 0.000 702.280 351.140 /home/astro/hsc/products/Linux64/meas_algorithms/HSC-3.8.0/python/lsst/meas/algorithms/detection.py:191(makeSourceCatalog)
 2 0.005 0.002 702.274 351.137 /home/astro/hsc/products/Linux64/meas_algorithms/HSC-3.8.0/python/lsst/meas/algorithms/detection.py:228(detectFootprints)
 15 0.001 0.000 698.597 46.573 /home/pprice/hsc/afw/python/lsst/afw/detection/detectionLib.py:3448(_init_)
 15 698.596 46.573 698.596 46.573 {_detectionLib.new_FootprintSet}
      [May-9 00:53] Paul Price: If I revert HSC-1243 ("Port better Footprint-grow code from LSST"), then the performance regression goes away. @jbosch @jds may be interested...

      The source of the regression must be identified and resolved for both HSC and LSST.

        Attachments

          Activity

          Hide
          price Paul Price added a comment -

          Glad we got to the bottom of this! Sorry it took longer than we would have liked...

          Show
          price Paul Price added a comment - Glad we got to the bottom of this! Sorry it took longer than we would have liked...
          Hide
          swinbank John Swinbank added a comment -

          Thanks for your help in tracking this down, Paul. Would you mind taking it on for review?

          There's just a single commit on afw's tickets/DM-2787. Since there are no functionality changes, this should already be covered by the existing unit tests.

          Show
          swinbank John Swinbank added a comment - Thanks for your help in tracking this down, Paul. Would you mind taking it on for review? There's just a single commit on afw 's tickets/DM-2787 . Since there are no functionality changes, this should already be covered by the existing unit tests.
          Hide
          price Paul Price added a comment -

          Nice code and style. Some minor comments:

          • The commit could use a more detailed commit message (e.g., what was the regression, how was it fixed?).
          • Should the iteration over foot.getSpans() and element use cbegin and cend?
          • I think xmin, xmax and yval should be const. Also start (but not end).
          • Since the Footprint so grown is normalised, if you kept track of the number of pixels and the bounding box as you went, you could set _normalized = true, which might be a boon to the user (especially since a call to getArea() immediately after calling this method would return the wrong value, since it doesn't check to see if the Footprint is normalised, and so the old area would be returned).
          Show
          price Paul Price added a comment - Nice code and style. Some minor comments: The commit could use a more detailed commit message (e.g., what was the regression, how was it fixed?). Should the iteration over foot.getSpans() and element use cbegin and cend ? I think xmin , xmax and yval should be const . Also start (but not end ). Since the Footprint so grown is normalised, if you kept track of the number of pixels and the bounding box as you went, you could set _normalized = true , which might be a boon to the user (especially since a call to getArea() immediately after calling this method would return the wrong value, since it doesn't check to see if the Footprint is normalised, and so the old area would be returned).
          Hide
          swinbank John Swinbank added a comment -

          Thanks for the speedy turnaround.

          I agree with all of your comments, except the last point re keeping track of the pixel count and bounding box. By my reading, the calls to addSpanInSeries() should automatically adjust the pixel count and bounding box as spans are added. The bounding box of the grown Footprint is already checked in tests/footprint1.py. I'll add another check to that test case to ensure the area is also correct.

          Show
          swinbank John Swinbank added a comment - Thanks for the speedy turnaround. I agree with all of your comments, except the last point re keeping track of the pixel count and bounding box. By my reading, the calls to addSpanInSeries() should automatically adjust the pixel count and bounding box as spans are added. The bounding box of the grown Footprint is already checked in tests/footprint1.py . I'll add another check to that test case to ensure the area is also correct.
          Hide
          price Paul Price added a comment -

          I think you're right.

          Show
          price Paul Price added a comment - I think you're right.

            People

            • Assignee:
              swinbank John Swinbank
              Reporter:
              swinbank John Swinbank
              Reviewers:
              Paul Price
              Watchers:
              Jim Bosch, John Swinbank, Paul Price, Robert Lupton
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel