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

replace "bad data" flag in SdssCentroid

    Details

      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.

        Attachments

          Activity

          Hide
          swinbank 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
          swinbank 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
          pgee 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
          pgee 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
          jbosch 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
          jbosch 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 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 Robyn Allsman [X] (Inactive) added a comment - I would like to fix this. Is it on master or still on a ticket branch?
          Hide
          jbosch Jim Bosch added a comment -

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

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

            People

            • Assignee:
              pgee Perry Gee
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              John Swinbank
              Watchers:
              Jim Bosch, John Swinbank, Perry Gee, Robyn Allsman [X] (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel