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

Write a version of the warper that uses SkyWcs and compare performance

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Story Points:
      20
    • Epic Link:
    • Sprint:
      Alert Production S17 - 5
    • Team:
      Alert Production

      Description

      Write a variant of the image warping functions that uses the new lsst::afw::geom::SkyWcs class. Compare performance with the old warper. It seems very likely we'll need to vectorize the transforms (of pixel position on the destination image to pixel position on the source) to get good performance, but confirm this.

        Attachments

          Activity

          Hide
          rowen Russell Owen added a comment - - edited

          Done. Thank you for the review. I have left the interpolated warping code as it was because I'm not sure how best to refactor it for simplicity. I did make one attempt using a class to hold information about interpolated rows (or columns) and it did clean things up a bit, but at the expense of storing data about interpolation rows that can be computed on the fly. Performance seemed fine but I worry about the additional memory required when warping large images with a small interpolation length. For now I left that code on a branch "u/rowen/temprefactor".

          Show
          rowen Russell Owen added a comment - - edited Done. Thank you for the review. I have left the interpolated warping code as it was because I'm not sure how best to refactor it for simplicity. I did make one attempt using a class to hold information about interpolated rows (or columns) and it did clean things up a bit, but at the expense of storing data about interpolation rows that can be computed on the fly. Performance seemed fine but I worry about the additional memory required when warping large images with a small interpolation length. For now I left that code on a branch "u/rowen/temprefactor".
          Hide
          rowen Russell Owen added a comment -

          I've responded to the comments and made a few changes, but did not try to refactor the warping code for reasons explained in my response to your comments. Would you like to have another look? I'd be happy to talk to you, as well, if you like.

          Show
          rowen Russell Owen added a comment - I've responded to the comments and made a few changes, but did not try to refactor the warping code for reasons explained in my response to your comments. Would you like to have another look? I'd be happy to talk to you, as well, if you like.
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Looks good, but some comments on PR. Main ones are about the complexity of the interpolation section and the confusing API for the direction of the transform.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Looks good, but some comments on PR. Main ones are about the complexity of the interpolation section and the confusing API for the direction of the transform.
          Hide
          rowen Russell Owen added a comment -

          Note: Transform does not yet offer an in-place tranForward method. It may be worth adding on as an experiment to see if it speeds up warping enough to bother with. But I suggest that be a different ticket, and not be done until after DM-9939 (stop transposing data in astshim), since that is going to be done and ought to speed things up a bit.

          Show
          rowen Russell Owen added a comment - Note: Transform does not yet offer an in-place tranForward method. It may be worth adding on as an experiment to see if it speeds up warping enough to bother with. But I suggest that be a different ticket, and not be done until after DM-9939 (stop transposing data in astshim), since that is going to be done and ought to speed things up a bit.
          Hide
          rowen Russell Owen added a comment -

          Implemented as a new overload to warpImage that uses a Transform<Point2Endpoint, Point2Endpoint> to describe the transformation.

          Once we replace our current Wcs class with lsst::afw::geom::SkyWcs this will be the code that the other overrides call. Until that time users will have to construct the transform manually (which is very easy if one has the SkyWcs for the source and destination exposure).

          There is a new test in tests/testWarpExposure.py that tests the code, and a new example examples/timeWarpExposureUsingTransform.py that times the new code. On my computer it runs a bit faster than the old code.

          Show
          rowen Russell Owen added a comment - Implemented as a new overload to warpImage that uses a Transform<Point2Endpoint, Point2Endpoint> to describe the transformation. Once we replace our current Wcs class with lsst::afw::geom::SkyWcs this will be the code that the other overrides call. Until that time users will have to construct the transform manually (which is very easy if one has the SkyWcs for the source and destination exposure). There is a new test in tests/testWarpExposure.py that tests the code, and a new example examples/timeWarpExposureUsingTransform.py that times the new code. On my computer it runs a bit faster than the old code.
          Hide
          tjenness Tim Jenness added a comment -

          Is it worth also experimenting with the AST internal warp codes?

          Show
          tjenness Tim Jenness added a comment - Is it worth also experimenting with the AST internal warp codes?

            People

            • Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Pim Schellart [X] (Inactive)
              Watchers:
              Pim Schellart [X] (Inactive), Russell Owen, Simon Krughoff, Tim Jenness
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel