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

DarkCombineTask broken

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: obs_subaru, pipe_drivers
    • Labels:
      None

      Description

      DM-8913 changed DarkCombineTask to use VisitInfo, but this assumed that the combined variable is an Exposure, but it's actually a DecoratedImage. That means we need to use a different means of getting the metadata in.

        Attachments

          Activity

          Hide
          price Paul Price added a comment -

          John Swinbank, would you please review this fix?

          pprice@tiger-sumire:~/LSST/pipe/drivers (tickets/DM-9055=) $ git sub-patch
          commit b38869003ba584709576bf4eee1b42777ae6e703
          Author: Paul Price <price@astro.princeton.edu>
          Date:   Thu Jan 19 17:40:15 2017 -0500
           
              DarkCombineTask: fix setting of metadata
              
              DM-8913 changed DarkCombineTask to use VisitInfo, but this assumed
              that the combined variable is an Exposure, but it's actually a
              DecoratedImage, which broke the dark creation. Fixed by using a
              different means to get the metadata in.
           
          diff --git a/python/lsst/pipe/drivers/constructCalibs.py b/python/lsst/pipe/drivers/constructCalibs.py
          index efea8fc..806f0ae 100644
          --- a/python/lsst/pipe/drivers/constructCalibs.py
          +++ b/python/lsst/pipe/drivers/constructCalibs.py
          @@ -649,7 +649,12 @@ class DarkCombineTask(CalibCombineTask):
               """Task to combine dark images"""
               def run(*args, **kwargs):
                   combined = CalibCombineTask.run(*args, **kwargs)
          -        combined.getInfo().setVisitInfo(afwImage.makeVisitInfo(exposureTime=1.0, darkTime=1.0))
          +
          +        # Update the metadata
          +        visitInfo = afwImage.makeVisitInfo(exposureTime=1.0, darkTime=1.0)
          +        md = dafBase.PropertyList.cast(combined.getMetadata())
          +        afwImage.setVisitInfoMetadata(md, visitInfo)
          +
                   return combined
           
           class DarkConfig(CalibConfig):
           
           
          pprice@tiger-sumire:~/LSST/obs/subaru (tickets/DM-9055=) $ git sub-patch
          commit 8c5efb3b7f89394382a97002c66381016c62f481
          Author: Paul Price <price@astro.princeton.edu>
          Date:   Thu Jan 19 17:42:04 2017 -0500
           
              config: adapt to removal of DarkConfig
              
              Darktime now comes from the VisitInfo rather than a header
              keyword.
           
          diff --git a/config/hsc/dark.py b/config/hsc/dark.py
          index 2da1bb6..e13408b 100644
          --- a/config/hsc/dark.py
          +++ b/config/hsc/dark.py
          @@ -4,8 +4,6 @@ from lsst.utils import getPackageDir
           
           config.load(os.path.join(getPackageDir("obs_subaru"), "config", "hsc", "isr.py"))
           
          -config.darkTime = None
          -
           config.isr.doBias = True
           config.repair.cosmicray.nCrPixelMax = 1000000
           config.repair.cosmicray.minSigma = 5.0
          

          Show
          price Paul Price added a comment - John Swinbank , would you please review this fix? pprice@tiger-sumire:~/LSST/pipe/drivers (tickets/DM-9055=) $ git sub-patch commit b38869003ba584709576bf4eee1b42777ae6e703 Author: Paul Price <price@astro.princeton.edu> Date: Thu Jan 19 17:40:15 2017 -0500   DarkCombineTask: fix setting of metadata DM-8913 changed DarkCombineTask to use VisitInfo, but this assumed that the combined variable is an Exposure, but it's actually a DecoratedImage, which broke the dark creation. Fixed by using a different means to get the metadata in.   diff --git a/python/lsst/pipe/drivers/constructCalibs.py b/python/lsst/pipe/drivers/constructCalibs.py index efea8fc..806f0ae 100644 --- a/python/lsst/pipe/drivers/constructCalibs.py +++ b/python/lsst/pipe/drivers/constructCalibs.py @@ -649,7 +649,12 @@ class DarkCombineTask(CalibCombineTask): """Task to combine dark images""" def run(*args, **kwargs): combined = CalibCombineTask.run(*args, **kwargs) - combined.getInfo().setVisitInfo(afwImage.makeVisitInfo(exposureTime=1.0, darkTime=1.0)) + + # Update the metadata + visitInfo = afwImage.makeVisitInfo(exposureTime=1.0, darkTime=1.0) + md = dafBase.PropertyList.cast(combined.getMetadata()) + afwImage.setVisitInfoMetadata(md, visitInfo) + return combined class DarkConfig(CalibConfig):     pprice@tiger-sumire:~/LSST/obs/subaru (tickets/DM-9055=) $ git sub-patch commit 8c5efb3b7f89394382a97002c66381016c62f481 Author: Paul Price <price@astro.princeton.edu> Date: Thu Jan 19 17:42:04 2017 -0500   config: adapt to removal of DarkConfig Darktime now comes from the VisitInfo rather than a header keyword.   diff --git a/config/hsc/dark.py b/config/hsc/dark.py index 2da1bb6..e13408b 100644 --- a/config/hsc/dark.py +++ b/config/hsc/dark.py @@ -4,8 +4,6 @@ from lsst.utils import getPackageDir config.load(os.path.join(getPackageDir("obs_subaru"), "config", "hsc", "isr.py")) -config.darkTime = None - config.isr.doBias = True config.repair.cosmicray.nCrPixelMax = 1000000 config.repair.cosmicray.minSigma = 5.0
          Hide
          swinbank John Swinbank added a comment -

          Looks broadly fine.

          Three comments:

          • When you set the metadata this way, you get a 'ROTTYPE': 'UNKNOWN' entry for free from VisitInfo. I can't imagine that's a problem, but it caught me by surprise.
          • Compared to the pre-DM-8913 way of setting the metadata, this looks pretty clunky. Other than the fact that we "ought to" use VisitInfo, are we actually gaining anything by doing this vs. just using the old (more obvious) way?
          • This seems like the sort of thing a test suite would have caught pretty easily. Given that this is fixing a bug impacting an end-user, I'm reluctant to make adding appropriate tests a requirement to getting this merged... but if you saw a quick & easy way to make that happen, it would be great.

          Assuming you are convinced that the answer to the second point is "yes", you can go ahead and merge this.

          Show
          swinbank John Swinbank added a comment - Looks broadly fine. Three comments: When you set the metadata this way, you get a 'ROTTYPE': 'UNKNOWN' entry for free from VisitInfo . I can't imagine that's a problem, but it caught me by surprise. Compared to the pre- DM-8913 way of setting the metadata, this looks pretty clunky. Other than the fact that we "ought to" use VisitInfo , are we actually gaining anything by doing this vs. just using the old (more obvious) way? This seems like the sort of thing a test suite would have caught pretty easily. Given that this is fixing a bug impacting an end-user, I'm reluctant to make adding appropriate tests a requirement to getting this merged... but if you saw a quick & easy way to make that happen, it would be great. Assuming you are convinced that the answer to the second point is "yes", you can go ahead and merge this.
          Hide
          price Paul Price added a comment -

          It's a bit more than just that we "ought to" use VisitInfo. The pre-DM-8913 way of setting the metadata assumed header keywords that I hoped would get picked up by afw. By using VisitInfo to write the headers, we can be sure that what we set is what will get picked up by afw (assuming that VisitInfo is internally consistent, of course, but that's a safer assumption than the original one).

          A test suite would be great. I've always been wary of making one because of the involvement of multiple cores, but I think we could imagine something using --batch-type=smp --cores=1 or even the new --batch-type=none. I've filed DM-9059 to create a test suite for pipe_drivers.

          Show
          price Paul Price added a comment - It's a bit more than just that we "ought to" use VisitInfo . The pre- DM-8913 way of setting the metadata assumed header keywords that I hoped would get picked up by afw. By using VisitInfo to write the headers, we can be sure that what we set is what will get picked up by afw (assuming that VisitInfo is internally consistent, of course, but that's a safer assumption than the original one). A test suite would be great. I've always been wary of making one because of the involvement of multiple cores, but I think we could imagine something using --batch-type=smp --cores=1 or even the new --batch-type=none . I've filed DM-9059 to create a test suite for pipe_drivers.
          Hide
          price Paul Price added a comment -

          Thanks John.

          Merged to master.

          Show
          price Paul Price added a comment - Thanks John. Merged to master.

            People

            • Assignee:
              price Paul Price
              Reporter:
              price Paul Price
              Reviewers:
              John Swinbank
              Watchers:
              John Swinbank, Paul Price
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel