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

Wrap skymap with pybind11

    Details

    • Story Points:
      2
    • Sprint:
      Alert Production S17 - 1
    • Team:
      Alert Production

      Description

      May not have any work associated with it, but is an lsst_distrib dependency. Investigate and update SP.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            One trivial change required in a unit test

            Show
            rowen Russell Owen added a comment - One trivial change required in a unit test
            Hide
            rowen Russell Owen added a comment -

            I decided to add the missing constructor to Point and Extent in afw

            Show
            rowen Russell Owen added a comment - I decided to add the missing constructor to Point and Extent in afw
            Hide
            rowen Russell Owen added a comment - - edited

            A test change that can be reverted after fixing afw is meas_astrom testMakeMatchStatistics in two places (look for (*).

            Show
            rowen Russell Owen added a comment - - edited A test change that can be reverted after fixing afw is meas_astrom testMakeMatchStatistics in two places (look for (* ).
            Hide
            rowen Russell Owen added a comment - - edited

            After wrapping the Point and Extent Eigen constructors in afw all tests in skymap passed.

            This change also allowed me to revert a modification to a test in meas_astrom.

            Finally I made some other changes in afw on separate commits (in case you don't want some of them) including (changes are in point.cc and extent.cc unless otherwise noted):

            • fix __neq__ -> __eq__ in several wrapper files (I will add these to my "needs test" list)
            • use namespace lsst { ... instead of using ...
            • stop using pybind11/operators.h
            • run clang-format, primarily to break some lines that were too long
            • add py::is_operator() in the remaining safe places (not in-place operators)

            Two other changes I would be happy to make in afw given your permission:

            • Get rid of a long note about SWIG in testCoordinates.py testInPlaceOperators that seems to be obsolete
            • Change the two wrapper files to use self and other instead of p/e and o for operator arguments
            Show
            rowen Russell Owen added a comment - - edited After wrapping the Point and Extent Eigen constructors in afw all tests in skymap passed. This change also allowed me to revert a modification to a test in meas_astrom . Finally I made some other changes in afw on separate commits (in case you don't want some of them) including (changes are in point.cc and extent.cc unless otherwise noted): fix __neq__ -> __eq__ in several wrapper files (I will add these to my "needs test" list) use namespace lsst { ... instead of using ... stop using pybind11/operators.h run clang-format , primarily to break some lines that were too long add py::is_operator() in the remaining safe places (not in-place operators) Two other changes I would be happy to make in afw given your permission: Get rid of a long note about SWIG in testCoordinates.py testInPlaceOperators that seems to be obsolete Change the two wrapper files to use self and other instead of p/e and o for operator arguments
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Looks good. Please feel free to finish cleanup as desired. But make sure py::is_operator does not change the expected behaviour when calling with non implemented type. The Point, Extent interactions were difficult to get right. Also please document any subtleties that you encounter such as when py::is_operator is deliberately not used.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Looks good. Please feel free to finish cleanup as desired. But make sure py::is_operator does not change the expected behaviour when calling with non implemented type. The Point , Extent interactions were difficult to get right. Also please document any subtleties that you encounter such as when py::is_operator is deliberately not used.
            Hide
            rowen Russell Owen added a comment -

            I rebased afw into 3 clean commits: fix __neq__, wrap the Eigen constructor of Point and Extent and modernize the point and extent wrappers. The latter includes standardizing the names for operators (which were quite disparate even within each file, and sometimes used "e" for a Point and "p" for an Extent) and removing the one SWIG comment in testCoordinates.py.

            Show
            rowen Russell Owen added a comment - I rebased afw into 3 clean commits: fix __neq__ , wrap the Eigen constructor of Point and Extent and modernize the point and extent wrappers. The latter includes standardizing the names for operators (which were quite disparate even within each file, and sometimes used "e" for a Point and "p" for an Extent) and removing the one SWIG comment in testCoordinates.py .

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                pschella Pim Schellart [X] (Inactive)
                Reviewers:
                Pim Schellart [X] (Inactive)
                Watchers:
                Jim Bosch, Krzysztof Findeisen, Pim Schellart [X] (Inactive), Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: