Details
-
Type:
Bug
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: obs_decam
-
Labels:
-
Team:DM Science
Description
Chris Morrison [X] and Meredith Rawls discovered that the updates to metadata handling uncovered a latent bug in DecamMapper.std_raw(), which used the header from HDU 1 to set the Exposure ID in VisitInfo instead of the header for the appropriate HDU. The fix is to use the correct HDU metadata.
Attachments
Activity
No pull request so I'm commenting here...
Since you're on it, may you please also correct the outdated comments in line 238-240 in decamMapper.py? Looks like that was the original reason to want the first HDU back then and got outdated with the makeRawVisitInfo work.
Also, a similar pattern of getting the "md0" exist in other places of the file. Is there more that could use a cleanup?
And, I'm not sure how good the DECam processing test coverage is on Jenkins today, so maybe somebody would want to run some manual tests or at least include lsst_ci
Hsin-Fang Chiang
added a comment - No pull request so I'm commenting here...
Since you're on it, may you please also correct the outdated comments in line 238-240 in decamMapper.py ? Looks like that was the original reason to want the first HDU back then and got outdated with the makeRawVisitInfo work.
Also, a similar pattern of getting the "md0" exist in other places of the file. Is there more that could use a cleanup?
And, I'm not sure how good the DECam processing test coverage is on Jenkins today, so maybe somebody would want to run some manual tests or at least include lsst_ci Never mind the very last point; lsst_ci is included by default.
Hsin-Fang Chiang
added a comment - Never mind the very last point; lsst_ci is included by default. I'm currently running the `ap_verify_hits2015` dataset using this ticket and have not seen any errors coming from this ticket. Colin Slater and I have confirmed that it produces the correct ExposureIds for the data.
Thanks for having a look, Hsin-Fang Chiang. Outdated comment removed. I'm all for getting rid of the other "md0" activity. I pushed a new commit that implements the same change (and a test) for instcals. For darks and cpMasterCals though it looks like we don't have test coverage of those, so I'm inclined to leave them as-is for the moment. Jenkins in progress.
Colin Slater
added a comment - Thanks for having a look, Hsin-Fang Chiang . Outdated comment removed. I'm all for getting rid of the other "md0" activity. I pushed a new commit that implements the same change (and a test) for instcals. For darks and cpMasterCals though it looks like we don't have test coverage of those, so I'm inclined to leave them as-is for the moment. Jenkins in progress. Merged to master.
Colin Slater
added a comment - Merged to master. 
Passed Centos jenkins, waiting on Mac OS. Giving to Chris Morrison [X] for review, but Hsin-Fang Chiang might also want to take a glance.