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
Colin Slater
created issue -
Colin Slater
made changes -
| Field | Original Value | New Value |
|---|---|---|
| Assignee | Colin Slater [ ctslater ] |
Colin Slater
made changes -
| Reviewers | Chris Morrison [ cmorrison ] | |
| Status | To Do [ 10001 ] | In Review [ 10004 ] |
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. | Status | In Review [ 10004 ] | Reviewed [ 10101 ] |
Merged to master.
Colin Slater
added a comment - Merged to master. Colin Slater
made changes -
| Resolution | Done [ 10000 ] | |
| Status | Reviewed [ 10101 ] | Done [ 10002 ] |
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.