Paul Price added a comment - 19/Jan/17 3:50 PM 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

John Swinbank added a comment - 19/Jan/17 4:19 PM 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.

Paul Price added a comment - 19/Jan/17 7:08 PM 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.

Paul Price added a comment - 19/Jan/17 7:11 PM Thanks John. Merged to master.