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

obs_decam should not call stripWcsKeywords

    XMLWordPrintable

    Details

    • Story Points:
      0.5
    • Epic Link:
    • Sprint:
      Alert Production S17 - 2
    • Team:
      Alert Production

      Description

      DecamMapper presently calls stripWcsKeywords in several places. This function is not part of afw's public interface (it is in namespace lsst::afw::image::detail and the call is unnecessary because makeWcs will strip the metadata if told to do so (by setting the second argument true).

      Fixing this will help the pybind11 conversion effort because the pybind11 wrapped version of afw does not make stripWcsKeywords available, and we'd rather not change that.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            Hsin-Fang Chiang do you have time to review this? It is a simple change, though I took the opportunity to also clean up some flake8 warnings (as a separate commit).

            Show
            rowen Russell Owen added a comment - Hsin-Fang Chiang do you have time to review this? It is a simple change, though I took the opportunity to also clean up some flake8 warnings (as a separate commit).
            Hide
            ctslater Colin Slater added a comment -

            This function is not part of afw's public interface (it is in namespace lsst::afw::image::detail)

            Since this is imperceptible from python, will pybind11 give us a better way to hide this function?

            Show
            ctslater Colin Slater added a comment - This function is not part of afw's public interface (it is in namespace lsst::afw::image::detail) Since this is imperceptible from python, will pybind11 give us a better way to hide this function?
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            A comment on the PR: may you modify the comments to reflect what makeWcs does now with an input TPV wcs? Those codes were there to workaround makeWcs.

            I'm not sure this part of obs_decam is covered well by the tests and CI. But may you please run validate_drp decam if Jenkins supports it now? (Jenkins didn't last time I heard... not sure how it is now..)

            Show
            hchiang2 Hsin-Fang Chiang added a comment - A comment on the PR: may you modify the comments to reflect what makeWcs does now with an input TPV wcs? Those codes were there to workaround makeWcs . I'm not sure this part of obs_decam is covered well by the tests and CI. But may you please run validate_drp decam if Jenkins supports it now? (Jenkins didn't last time I heard... not sure how it is now..)
            Hide
            rowen Russell Owen added a comment - - edited

            Colin Slater pybind11 will not wrap stripWcsKeywords at all if we can manage it. If we do wrap it, we should put it into lsst.afw.image.detail instead of lsst.afw.image (and that is where it should have been all along, but it's not easy to do that given the way the C++ code is written).

            Hsin-Fang Chiang I think the unit tests do a pretty good job of making sure that keywords have been stripped but running validate_drp on DECam is a good idea. I will do that.

            Show
            rowen Russell Owen added a comment - - edited Colin Slater pybind11 will not wrap stripWcsKeywords at all if we can manage it. If we do wrap it, we should put it into lsst.afw.image.detail instead of lsst.afw.image (and that is where it should have been all along, but it's not easy to do that given the way the C++ code is written). Hsin-Fang Chiang I think the unit tests do a pretty good job of making sure that keywords have been stripped but running validate_drp on DECam is a good idea. I will do that.
            Hide
            rowen Russell Owen added a comment - - edited

            I ran the quick DECam demo in validate_drp as far as I could (it died on the validation step) with and without this change. The resulting calexp and icExp were identical in both cases. So I think that proves this change is indeed innocuous (as expected, since calling makeWcs with the second argument true simply calls stripWcsMetadata, thus having the same effect as the current code.

            Show
            rowen Russell Owen added a comment - - edited I ran the quick DECam demo in validate_drp as far as I could (it died on the validation step) with and without this change. The resulting calexp and icExp were identical in both cases. So I think that proves this change is indeed innocuous (as expected, since calling makeWcs with the second argument true simply calls stripWcsMetadata , thus having the same effect as the current code.
            Hide
            rowen Russell Owen added a comment -

            I updated clarified the one comment and merged to master.

            Show
            rowen Russell Owen added a comment - I updated clarified the one comment and merged to master.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Hsin-Fang Chiang
              Watchers:
              Colin Slater, Hsin-Fang Chiang, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.