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

forcedPhotCoadd.py fails on CFHT data due to a CModel bug

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_modelfit
    • Labels:
      None
    • Story Points:
      1
    • Epic Link:
    • Sprint:
      DRP F16-2
    • Team:
      Data Release Production

      Description

      Hello,

      forcedPhotCoadd fails while running on CFHT data due to a CModel bug. Here is an example on the error message that we get:

      python: src/CModel.cc:1368: void lsst::meas::modelfit::CModelAlgorithm::measure(lsst::afw::table::SourceRecord&, const lsst::afw::image::Exposure<float>&, const lsst::afw::table::SourceRecord&) const: Assertion `measRecord.getFootprint()->getArea()' failed.
      Aborted
      

      Adding the following lines in cmodel.py (in CModelForcedPlugin.measure, before the call to self.algorithm.measure) allows to go around the problem for the time being, which seems to arise for null value of the number of pixel in a given footprint:

      if not measRecord.getFootprint().getArea():
          raise ValueError("measRecord.getFootprint().getArea(): 0. No pixel in this footprint.")
      

        Attachments

          Activity

          Hide
          jbosch Jim Bosch added a comment -

          Lauren MacArthur, trivial review for you: remove a single unhelpful assert, and add a line testing that it's gone. Here's the full change:

          commit a8c2344de88b66affd4630bc088948a8dc11291f
          Author: Jim Bosch <jbosch@astro.princeton.edu>
          Date:   Tue Jul 5 10:28:51 2016 -0400
           
              Remove unnecessary assert on footprint area.
              
              The new pixel region code should handle this condition gracefully
              already (by setting the usedPsfArea flag).
           
          diff --git a/src/CModel.cc b/src/CModel.cc
          index b9e1567..dd3cc8f 100644
          --- a/src/CModel.cc
          +++ b/src/CModel.cc
          @@ -1365,7 +1365,6 @@ void CModelAlgorithm::measure(
               afw::table::SourceRecord const & refRecord
           ) const {
               Result result = _impl->makeResult();
          -    assert(measRecord.getFootprint()->getArea());
               // Read the shapelet approximation to the PSF, load/verify other inputs from the SourceRecord
               shapelet::MultiShapeletFunction psf = _processInputs(measRecord, exposure);
               // If PsfFlux has been run, use that for approx flux; otherwise we'll compute it ourselves.
          diff --git a/tests/testCModelPlugins.py b/tests/testCModelPlugins.py
          index 2c77f1e..27b1585 100755
          --- a/tests/testCModelPlugins.py
          +++ b/tests/testCModelPlugins.py
          @@ -106,6 +106,8 @@ class CModelTestCase(AlgorithmTestCase):
                   refWcs = exposure1.getWcs()
                   refCat = catalog1
                   measCat = forcedTask.generateMeasCat(exposure2, refCat, refWcs)
          +        # Check that we don't require footprints in forced photometry
          +        measCat[0].setFootprint(None)
                   forcedTask.attachTransformedFootprints(measCat, refCat, exposure2, refWcs)
                   forcedTask.run(measCat, exposure2, refCat, refWcs)
                   self.checkOutputs(measCat, catalog2)
          

          Show
          jbosch Jim Bosch added a comment - Lauren MacArthur , trivial review for you: remove a single unhelpful assert, and add a line testing that it's gone. Here's the full change: commit a8c2344de88b66affd4630bc088948a8dc11291f Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Tue Jul 5 10:28:51 2016 -0400   Remove unnecessary assert on footprint area. The new pixel region code should handle this condition gracefully already (by setting the usedPsfArea flag).   diff --git a/src/CModel.cc b/src/CModel.cc index b9e1567..dd3cc8f 100644 --- a/src/CModel.cc +++ b/src/CModel.cc @@ -1365,7 +1365,6 @@ void CModelAlgorithm::measure( afw::table::SourceRecord const & refRecord ) const { Result result = _impl->makeResult(); - assert(measRecord.getFootprint()->getArea()); // Read the shapelet approximation to the PSF, load/verify other inputs from the SourceRecord shapelet::MultiShapeletFunction psf = _processInputs(measRecord, exposure); // If PsfFlux has been run, use that for approx flux; otherwise we'll compute it ourselves. diff --git a/tests/testCModelPlugins.py b/tests/testCModelPlugins.py index 2c77f1e..27b1585 100755 --- a/tests/testCModelPlugins.py +++ b/tests/testCModelPlugins.py @@ -106,6 +106,8 @@ class CModelTestCase(AlgorithmTestCase): refWcs = exposure1.getWcs() refCat = catalog1 measCat = forcedTask.generateMeasCat(exposure2, refCat, refWcs) + # Check that we don't require footprints in forced photometry + measCat[0].setFootprint(None) forcedTask.attachTransformedFootprints(measCat, refCat, exposure2, refWcs) forcedTask.run(measCat, exposure2, refCat, refWcs) self.checkOutputs(measCat, catalog2)
          Hide
          jbosch Jim Bosch added a comment -

          Ack, nevermind on the review for now. Found a problem myself. Will ping you when it's actually ready.

          Show
          jbosch Jim Bosch added a comment - Ack, nevermind on the review for now. Found a problem myself. Will ping you when it's actually ready.
          Hide
          jbosch Jim Bosch added a comment -

          Ok, now it's ready. I just removed the test code, since it wasn't actually doing anything, and I convinced myself that the test isn't needed (if you look at the function where I removed the assert, you can see that the Footprint isn't actually being passed to any other code). I also updated the commit message to reflect that.

          Here's the new change:

          commit 07f6f13fa320b6a30f5a7989f2cfa171c86f8155
          Author: Jim Bosch <jbosch@astro.princeton.edu>
          Date:   Tue Jul 5 10:28:51 2016 -0400
           
              Remove unnecessary assert on footprint area.
              
              Forced CModel doesn't even use the attached Footprint at all anymore,
              as it uses the region ellipses instead.
           
          diff --git a/src/CModel.cc b/src/CModel.cc
          index b9e1567..dd3cc8f 100644
          --- a/src/CModel.cc
          +++ b/src/CModel.cc
          @@ -1365,7 +1365,6 @@ void CModelAlgorithm::measure(
               afw::table::SourceRecord const & refRecord
           ) const {
               Result result = _impl->makeResult();
          -    assert(measRecord.getFootprint()->getArea());
               // Read the shapelet approximation to the PSF, load/verify other inputs from the SourceRecord
               shapelet::MultiShapeletFunction psf = _processInputs(measRecord, exposure);
               // If PsfFlux has been run, use that for approx flux; otherwise we'll compute it ourselves.
          

          Show
          jbosch Jim Bosch added a comment - Ok, now it's ready. I just removed the test code, since it wasn't actually doing anything, and I convinced myself that the test isn't needed (if you look at the function where I removed the assert, you can see that the Footprint isn't actually being passed to any other code). I also updated the commit message to reflect that. Here's the new change: commit 07f6f13fa320b6a30f5a7989f2cfa171c86f8155 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Tue Jul 5 10:28:51 2016 -0400   Remove unnecessary assert on footprint area. Forced CModel doesn't even use the attached Footprint at all anymore, as it uses the region ellipses instead.   diff --git a/src/CModel.cc b/src/CModel.cc index b9e1567..dd3cc8f 100644 --- a/src/CModel.cc +++ b/src/CModel.cc @@ -1365,7 +1365,6 @@ void CModelAlgorithm::measure( afw::table::SourceRecord const & refRecord ) const { Result result = _impl->makeResult(); - assert(measRecord.getFootprint()->getArea()); // Read the shapelet approximation to the PSF, load/verify other inputs from the SourceRecord shapelet::MultiShapeletFunction psf = _processInputs(measRecord, exposure); // If PsfFlux has been run, use that for approx flux; otherwise we'll compute it ourselves.
          Hide
          lauren Lauren MacArthur added a comment -

          Heh, you addressed both questions I was pondering: 1) are you sure passing in just source [0] with no footprint is guaranteed to do anything, and 2) the word "should" in the commit message was not very comforting.

          Looks good.

          Show
          lauren Lauren MacArthur added a comment - Heh, you addressed both questions I was pondering: 1) are you sure passing in just source [0] with no footprint is guaranteed to do anything, and 2) the word "should" in the commit message was not very comforting. Looks good.
          Hide
          jbosch Jim Bosch added a comment -

          Merged to master.

          Show
          jbosch Jim Bosch added a comment - Merged to master.

            People

            • Assignee:
              jbosch Jim Bosch
              Reporter:
              nchotard Nicolas Chotard
              Reviewers:
              Lauren MacArthur
              Watchers:
              Dominique Boutigny, Jim Bosch, John Swinbank, Lauren MacArthur, Nicolas Chotard
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: