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

Decam VisitInfo ExposureId set incorrectly

    XMLWordPrintable

Details

    • DM Science

    Description

      cmorrison and mrawls 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

          ctslater Colin Slater added a comment -

          Passed Centos jenkins, waiting on Mac OS. Giving to cmorrison for review, but hchiang2 might also want to take a glance.

          ctslater Colin Slater added a comment - Passed Centos jenkins, waiting on Mac OS. Giving to cmorrison for review, but hchiang2 might also want to take a glance.

          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

          hchiang2 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.

          hchiang2 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. ctslater and I have confirmed that it produces the correct ExposureIds for the data.

          cmorrison Chris Morrison [X] (Inactive) added a comment - I'm currently running the `ap_verify_hits2015` dataset using this ticket and have not seen any errors coming from this ticket. ctslater and I have confirmed that it produces the correct ExposureIds for the data.
          ctslater Colin Slater added a comment -

          Thanks for having a look, hchiang2. 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.

          ctslater Colin Slater added a comment - Thanks for having a look, hchiang2 . 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.
          ctslater Colin Slater added a comment -

          Merged to master.

          ctslater Colin Slater added a comment - Merged to master.

          People

            ctslater Colin Slater
            ctslater Colin Slater
            Chris Morrison [X] (Inactive)
            Chris Morrison [X] (Inactive), Colin Slater, Hsin-Fang Chiang
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Jenkins

                No builds found.