The code submitted here is fine, I think – I made one tiny suggestion on the pull request, but there's nothing substantive.
However, there are two other things to consider before you merge. One is that you re-write the commit message bearing in mind the advice on Confluence – the current message is too long for the initial line and also contains a misplaced apostrophe. We also don't normally mention the ticket number at the start of the message, although I don't think that's an important concern.
Secondly, I think it would be a good idea to think again about writing some tests. I note your comment above, but I don't see why writing some simple tests should be too difficult. To get you started, I hacked together a simple test of NO_2ND_DERIVATIVE:
def testFlagNo2ndDeriv(self):
|
self.truth.defineCentroid("truth")
|
centroid = self.truth[0].getCentroid()
|
psfImage = self.calexp.getPsf().computeImage(centroid)
|
# construct a box that won't fit the full PSF model
|
bbox = psfImage.getBBox()
|
bbox.grow(10)
|
subImage = lsst.afw.image.ExposureF(self.calexp, bbox)
|
subImage.getMaskedImage().getImage().getArray()[:] = 0
|
subCat = self.measCat[:1]
|
# we also need to install a smaller footprint, or NoiseReplacer complains before we even get to
|
# measuring the centriod
|
measRecord = subCat[0]
|
newFootprint = lsst.afw.detection.Footprint(bbox)
|
newFootprint.getPeaks().push_back(measRecord.getFootprint().getPeaks()[0])
|
measRecord.setFootprint(newFootprint)
|
# just measure the one object we've prepared for
|
self.task.measure(subCat, subImage)
|
self.assertTrue(measRecord.get("base_SdssCentroid_flag"))
|
self.assertTrue(measRecord.get("base_SdssCentroid_flag_no_2nd_derivative"))
|
Note that "hacked" is really the appropriate word here – I just grabbed the extant testEdge() and butchered that into something that does the right thing; you should definitely clean it up before committing it. Doing something similar for the other flags shouldn't be too hard.
The code submitted here is fine, I think – I made one tiny suggestion on the pull request, but there's nothing substantive.
However, there are two other things to consider before you merge. One is that you re-write the commit message bearing in mind the advice on Confluence – the current message is too long for the initial line and also contains a misplaced apostrophe. We also don't normally mention the ticket number at the start of the message, although I don't think that's an important concern.
Secondly, I think it would be a good idea to think again about writing some tests. I note your comment above, but I don't see why writing some simple tests should be too difficult. To get you started, I hacked together a simple test of NO_2ND_DERIVATIVE:
measRecord.setFootprint(newFootprint)
Note that "hacked" is really the appropriate word here – I just grabbed the extant testEdge() and butchered that into something that does the right thing; you should definitely clean it up before committing it. Doing something similar for the other flags shouldn't be too hard.