Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw
-
Labels:None
-
Story Points:5
-
Epic Link:
-
Sprint:Science Pipelines DM-S15-3
-
Team:Data Release Production
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
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.
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).
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).
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.
I think you're right.
Paul Price
added a comment - I think you're right.
Glad we got to the bottom of this! Sorry it took longer than we would have liked...