John Swinbank added a comment - 11/Jun/15 4:25 PM I'm a little confused as to what the situation really is here. Russell Owen seems to think the existing code is "clearly broken", but Paul Price is not sure whether the existing code or the proposed change is correct. Is that right? If so, perhaps Russell Owen could get things moving providing a more detailed motivation for the change and sending it to Paul Price for review. Would that do the trick? Perhaps that detailed explanation will help shake out a clearer way of expressing the code – or, failing that, the explanation itself can be added to the code as a comment.

Russell Owen added a comment - 12/Jun/15 9:57 AM Here is the code block in context: # The bounding-box of the local region we are going to fit ("stamp") xlo = int(math.floor(cx - R1)) ylo = int(math.floor(cy - R1)) xhi = int(math.ceil (cx + R1)) yhi = int(math.ceil (cy + R1)) stampbb = afwGeom.Box2I(afwGeom.Point2I(xlo, ylo), afwGeom.Point2I(xhi, yhi)) stampbb.clip(fbb) xlo, xhi = stampbb.getMinX(), stampbb.getMaxX() ylo, yhi = stampbb.getMinY(), stampbb.getMaxY() if xlo > xhi or ylo > yhi: log.logdebug('Skipping this peak: out of bounds') pkres.setOutOfBounds() return   # drop tiny footprints too? if tinyFootprintSize>0 and (((xhi - xlo) < tinyFootprintSize) or ((yhi - ylo) < tinyFootprintSize)): log.logdebug('Skipping this peak: tiny footprint / close to edge') pkres.setTinyFootprint() return   # find other peaks within range... otherpeaks = [] for pk2, pkF2 in zip(peaks, peaksF): if pk2 == pk: continue if pkF.distanceSquared(pkF2) > R2**2: continue opsfimg = psf.computeImage(pkF2.getX(), pkF2.getY()) if not opsfimg.getBBox(afwImage.LOCAL).overlaps(stampbb): continue otherpeaks.append(opsfimg) log.logdebug('%i other peaks within range' % len(otherpeaks)) The original code (before we switched the default for getBBox from LOCAL to PARENT) was identical except the word "LOCAL" was missing in the line "if not opsfimg.getBBox(afwImage.LOCAL).overlaps(stampbb)". So this code behaves exactly as it did before the default for getBBox was changed. But the code is clearly broken! opsfimg.getBBox(LOCAL) will always be a bounding box whose minimum/lower left corner is always 0,0, whereas what the code claims to by trying to determine is if the opsfimg overlaps the stamp, whose bounding box almost never have a min of 0,0. In my opinion LOCAL should always have been PARENT and the existing code was patently incorrect. My concern is as follows: if I fix this bug then what happens? The code never worked as designed. Thus that code was never properly tested. What if it has some other bug that will be exposed by fixing this obvious bug? I think somebody who has a better sense of what's going on should review the code and decide what to do. That is why I handed it over to Robert originally. That is still what I think needs to be done. So I agree with Paul: somebody who knows that's going on should decide how to proceed. A unit test that actually exercises this bit of code would also be really helpful.

Russell Owen added a comment - 12/Jun/15 10:00 AM One more thing said above, but it bears repeating: all our existing unit tests and our integration test pass whether this bug is fixed or not. At some level that is comforting: it appears to be a minor change. But it's still worrisome. Do we need this code at all? If so, how can we exercise it so we're confident it is working.

Tim Jenness added a comment - 12/Jun/15 12:16 PM The meas_deblender test suite does execute that line of code and sometimes the if statement returns True and other times it returns False (both branches show coverage).

John Swinbank added a comment - 26/Mar/19 3:20 PM Fixed on DM-10574 .