# replace "bad data" flag in SdssCentroid

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
3
• Sprint:
Science Pipelines DM-S15-1
• Team:
Data Release Production

#### Description

SdssCentroid has a "bad data" flag that doesn't actually convey any information about what went wrong. This should be replaced with one or more flags that provide more information.

#### Activity

Hide
John Swinbank added a comment -

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.

Show
John Swinbank added a comment - 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.
Hide
Perry Gee added a comment -

This is a perfectly reasonable request. Since I didn't really understand the reason for the almost_no_2nd_derivative error, I thought it might be better for someone who was actually writing this algorithm to come up with test cases. But I certainly can make an artificial test which produces the error by just looking at the failure case.

Show
Perry Gee added a comment - This is a perfectly reasonable request. Since I didn't really understand the reason for the almost_no_2nd_derivative error, I thought it might be better for someone who was actually writing this algorithm to come up with test cases. But I certainly can make an artificial test which produces the error by just looking at the failure case.
Hide
Jim Bosch added a comment -

While resolving conflicts between this and DM-1161 during a rebase, I noticed that the test for almost_no_2nd_derivative wasn't actually doing what it claimed to be doing; the test for that particular flag had been commented out, and the main failure flag was only set because the "edge" condition was being hit.

After a lot of fiddling with the test code and trying to reproduce that error condition, I've basically given up, and just removed that test in a commit on DM-1161. Unless Perry Gee actually has a prescription in hand for triggering it (and it just got left off this issue by mistake), I'm content to leave it that way.

I also went ahead and renamed the new flags on DM-1161 after resolving the conflicts and getting the rest of the test code working again with my changes to the test utility code. The schema naming conventions use underscores as a "group" separator, with camelCase used as a word separator - so it should be "base_SdssCentroid_flag_notAtMaximum" instead of "base_SdssCentroid_flag_not_at_maximum".

Show
Jim Bosch added a comment - While resolving conflicts between this and DM-1161 during a rebase, I noticed that the test for almost_no_2nd_derivative wasn't actually doing what it claimed to be doing; the test for that particular flag had been commented out, and the main failure flag was only set because the "edge" condition was being hit. After a lot of fiddling with the test code and trying to reproduce that error condition, I've basically given up, and just removed that test in a commit on DM-1161 . Unless Perry Gee actually has a prescription in hand for triggering it (and it just got left off this issue by mistake), I'm content to leave it that way. I also went ahead and renamed the new flags on DM-1161 after resolving the conflicts and getting the rest of the test code working again with my changes to the test utility code. The schema naming conventions use underscores as a "group" separator, with camelCase used as a word separator - so it should be "base_SdssCentroid_flag_notAtMaximum" instead of "base_SdssCentroid_flag_not_at_maximum".
Hide
Robyn Allsman [X] (Inactive) added a comment -

I would like to fix this. Is it on master or still on a ticket branch?

Show
Robyn Allsman [X] (Inactive) added a comment - I would like to fix this. Is it on master or still on a ticket branch?
Hide
Jim Bosch added a comment -

It's on master now; sorry. Probably best to just open a new ticket.

Show
Jim Bosch added a comment - It's on master now; sorry. Probably best to just open a new ticket.

#### People

Assignee:
Perry Gee
Reporter:
Jim Bosch
Reviewers:
John Swinbank
Watchers:
Jim Bosch, John Swinbank, Perry Gee, Robyn Allsman [X] (Inactive)