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

FrameSet frames not preserved by Transform(frameSet) constructor

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw

      Description

      When an lsst.afw.geom.Transform is constructed from an astshim.FrameSet the internal frames in the frame set are lost. I strongly suspect what is happening is that the mapping constructor of Transform is being called, instead of the FrameSet constructor. Reversing the order in the pybind11 wrapper file should fix the problem. A unit test is required.

        Attachments

          Issue Links

          is blocked by

          Bug - A problem which impairs or prevents the functions of the product. DM-9899 FrameSet.permAxes will fail if nIn != nOut

          • Done

            Activity

            Ascending order - Click to sort in descending order
            No builds found.
            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Epic Link DM-9680 [ 30785 ]
            rowen Russell Owen made changes -
            Summary FrameSet frames no preserved by Transform(frameSet) constructor FrameSet frames not preserved by Transform(frameSet) constructor
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-9899 [ DM-9899 ]
            rowen Russell Owen made changes -
            Story Points 2 1
            Hide
            Permalink
            rowen Russell Owen added a comment - - edited

            Transform(FrameSet) was never called in Python, because
            Transform(Mapping) was listed first in the pybind11 wrapper,
            and so was always called.
            Fixing this turned up several bugs in normalizing frames:

            • naive tests in PointEndpoint and SpherePointEndpoint
              were fooled when a FrameSet was passed in as a Frame.
            • SpherePointEndpoint needed a tweak so that permAxes
              was called on the frame argument, rather than a copy
              when the frame was a FrameSet.

            I added tests for all this in tests/test_transform.py. I'm afraid that file is getting rather large, but I'm not sure what to do about it. Perhaps some refactoring could make it smaller.

            I also replaced all usage of self.assertTrue(numpy.allclose(...)) with numpy.testing.assert_allclose(...) as a separate commit.

            Show
            rowen Russell Owen added a comment - - edited Transform(FrameSet) was never called in Python, because Transform(Mapping) was listed first in the pybind11 wrapper, and so was always called. Fixing this turned up several bugs in normalizing frames: naive tests in PointEndpoint and SpherePointEndpoint were fooled when a FrameSet was passed in as a Frame. SpherePointEndpoint needed a tweak so that permAxes was called on the frame argument, rather than a copy when the frame was a FrameSet. I added tests for all this in tests/test_transform.py . I'm afraid that file is getting rather large, but I'm not sure what to do about it. Perhaps some refactoring could make it smaller. I also replaced all usage of self.assertTrue(numpy.allclose(...)) with numpy.testing.assert_allclose(...) as a separate commit.
            rowen Russell Owen made changes -
            Reviewers Krzysztof Findeisen [ krzys ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            Hide
            Permalink
            krzys Krzysztof Findeisen added a comment -

            I requested a few changes on GitHub, but overall looks nice. I especially like the extra testing infrastructure you added to test_transform.py.

            Show
            krzys Krzysztof Findeisen added a comment - I requested a few changes on GitHub, but overall looks nice. I especially like the extra testing infrastructure you added to test_transform.py .
            krzys Krzysztof Findeisen made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            Permalink
            rowen Russell Owen added a comment -

            Thank you for your very helpful review. I feel test_transform.py is much better based on your suggestions and some of the documentation should also be clearer. I have rebased the changes and will merge to master once a Jenkins run passes.

            Show
            rowen Russell Owen added a comment - Thank you for your very helpful review. I feel test_transform.py is much better based on your suggestions and some of the documentation should also be clearer. I have rebased the changes and will merge to master once a Jenkins run passes.
            rowen Russell Owen made changes -
            Story Points 1 2
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Krzysztof Findeisen
              Watchers:
              Krzysztof Findeisen, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.