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

Add wrapper on astshim to take point lists

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      20
    • Sprint:
      Alert Production S17 - 2, Alert Production S17 - 3
    • Team:
      Alert Production

      Description

      Once the new point classes are implemented in DM-1987 this will provide the wrapper on top of astshim to pass them in and get them out both as lists and as single points.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            Thanks for agreeing to review this. This includes:

            • A few new constructors for SpherePoint which we don't necessarily need, but which are convenient.
            • New Endpoint classes that act as interfaces between the data used by astshim and our specialized classes Point<N>D and SpherePoint.
            • The start of a new class Transform which holds two Endpoints and an ast::FrameSet. Only tranForward and tranInverse are supported on this ticket. There are linked tickets to add additional behavior such as returning the Jacobian, making an inverse transform and making a compound transform.
            Show
            rowen Russell Owen added a comment - Thanks for agreeing to review this. This includes: A few new constructors for SpherePoint which we don't necessarily need, but which are convenient. New Endpoint classes that act as interfaces between the data used by astshim and our specialized classes Point<N>D and SpherePoint . The start of a new class Transform which holds two Endpoints and an ast::FrameSet . Only tranForward and tranInverse are supported on this ticket. There are linked tickets to add additional behavior such as returning the Jacobian, making an inverse transform and making a compound transform.
            Hide
            rowen Russell Owen added a comment -

            I believe I have addressed all the review issues, including:

            • Removing the default constructor for SpherePoint
            • Improve the documentation and comments as requested
            • Add default copy and move constructors to Endpoint
            • Add default move constructors to Transform and disable copy constructors
            • Remove one unused import from the Endpoint and Transform pybind11 wrappers

            I addition I made the following changes:

            • Improved the unit tests as requested.
            • Add ostream::operator<< to the C++ classes and __str__ and __repr__ to the Python wrappers. The latter have unit tests. Also the Endpoint version of ostream::operator<< is used by Endpoint.__str__ and __repr__ so it is tested. The Transform version of ostream::operator<< is not used by the Python wrapper (since the templated class name naturally looks different than the C++ class name) and is not tested.
            • Fixed a few minor pep8 errors in the new unit tests (my linter was broken when I wrote those tests)

            Please have another look.

            Show
            rowen Russell Owen added a comment - I believe I have addressed all the review issues, including: Removing the default constructor for SpherePoint Improve the documentation and comments as requested Add default copy and move constructors to Endpoint Add default move constructors to Transform and disable copy constructors Remove one unused import from the Endpoint and Transform pybind11 wrappers I addition I made the following changes: Improved the unit tests as requested. Add ostream::operator<< to the C++ classes and __str__ and __repr__ to the Python wrappers. The latter have unit tests. Also the Endpoint version of ostream::operator<< is used by Endpoint.__str__ and __repr__ so it is tested. The Transform version of ostream::operator<< is not used by the Python wrapper (since the templated class name naturally looks different than the C++ class name) and is not tested. Fixed a few minor pep8 errors in the new unit tests (my linter was broken when I wrote those tests) Please have another look.
            Hide
            rowen Russell Owen added a comment -

            Also I made each change its own commit (in hopes of making the followup review easier) and separated out work on Endpoint and Transform (to simplify a planned squash). I intend to squash the commits after the review to leave 1 commit each for SpherePoint, Endpoint and Transform.

            Show
            rowen Russell Owen added a comment - Also I made each change its own commit (in hopes of making the followup review easier) and separated out work on Endpoint and Transform (to simplify a planned squash). I intend to squash the commits after the review to leave 1 commit each for SpherePoint, Endpoint and Transform.
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for the cleanups. It's unfortunate that we can't use assertFloatsAlmostEqual here, but I guess them's the breaks.

            Show
            Parejkoj John Parejko added a comment - Thanks for the cleanups. It's unfortunate that we can't use assertFloatsAlmostEqual here, but I guess them's the breaks.
            Hide
            rowen Russell Owen added a comment -

            As per John Parejko's request I'm asking Krzysztof Findeisen to have a look at the pybind11 (and anything else of interest).

            Show
            rowen Russell Owen added a comment - As per John Parejko 's request I'm asking Krzysztof Findeisen to have a look at the pybind11 (and anything else of interest).
            Hide
            krzys Krzysztof Findeisen added a comment -

            The pybind11 wrappers look good; I did request some minor changes on GitHub.

            Show
            krzys Krzysztof Findeisen added a comment - The pybind11 wrappers look good; I did request some minor changes on GitHub.
            Hide
            rowen Russell Owen added a comment -

            Merged. AST is now officially part of the DM stack.

            Show
            rowen Russell Owen added a comment - Merged. AST is now officially part of the DM stack.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                krughoff Simon Krughoff
                Reviewers:
                Krzysztof Findeisen
                Watchers:
                John Parejko, Krzysztof Findeisen, Russell Owen, Simon Krughoff, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel