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

Wrap afw::cameraGeom with pybind11

    XMLWordPrintable

    Details

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

      Description

      The generated wrappers will live parallel to the Swig wrappers. This ticket only covers the C++ wrappers themselves, not the Python layer on top (which will continue to use the old wrappers) all work will stay on a separate branch and will not be merged to master until DM-6168 is complete.

      The tests included in this ticket are:

      1. testDetector.py
      2. testMakePixelToTanPixel.py
      3. testExposure.py
      4. testCamGeomFitsUtils.py
      5. testCameraGeom.py
      6. testDistortedTanWcs.py
      7. testCameraTransformMap.py
      8. testColor.py
      9. testOrientation.py
      10. testCameraSys.py
      11. testWarper.py (moved to DM-8619)

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            It appears that DM-8420, DM-8422, and DM-8423 all require FOCAL_PLANE to be defined.

            Show
            krzys Krzysztof Findeisen added a comment - - edited It appears that DM-8420 , DM-8422 , and DM-8423 all require FOCAL_PLANE to be defined.
            Hide
            rowen Russell Owen added a comment -

            I moved testWarper.py to a new ticket DM-8619 because I felt modifying afw.math was out of scope for cameraGeom work.

            Show
            rowen Russell Owen added a comment - I moved testWarper.py to a new ticket DM-8619 because I felt modifying afw.math was out of scope for cameraGeom work.
            Hide
            rowen Russell Owen added a comment -

            `testCameraGeom.py` fails in Python 2 because `CameraSys.getName` returns unicode, and that test uses such a value as a key in a `pex.config.ConfigDictField` and pex_config rejects that. See DM-8626.

            I may have a fix for that (which will require no changes to afw). Depending on how quickly I can get that fixed I will either disable `testCameraGeom.py` or get the fix merged to pex_config DM-8467 before merging this ticket and verify that the all tests pass.

            Show
            rowen Russell Owen added a comment - `testCameraGeom.py` fails in Python 2 because `CameraSys.getName` returns unicode, and that test uses such a value as a key in a `pex.config.ConfigDictField` and pex_config rejects that. See DM-8626 . I may have a fix for that (which will require no changes to afw). Depending on how quickly I can get that fixed I will either disable `testCameraGeom.py` or get the fix merged to pex_config DM-8467 before merging this ticket and verify that the all tests pass.
            Hide
            krzys Krzysztof Findeisen added a comment -

            I have a bunch of minor comments, but nothing show-stopping. Feel free to merge after resolving the comments.

            Show
            krzys Krzysztof Findeisen added a comment - I have a bunch of minor comments, but nothing show-stopping. Feel free to merge after resolving the comments.
            Hide
            rowen Russell Owen added a comment -

            Thank you for the very helpful review. Regarding symbols in lsst::afw::image::detail I am emulating SWIG for now but filed DM-8651 to improve it later. Regarding missing @brief I fixed the doxygen config (on DM-8647) so it's no longer needed. I believe I adopted all your other suggested changes (except a few that were questions "is this really needed?" to which the answer was "yes").

            Show
            rowen Russell Owen added a comment - Thank you for the very helpful review. Regarding symbols in lsst::afw::image::detail I am emulating SWIG for now but filed DM-8651 to improve it later. Regarding missing @brief I fixed the doxygen config (on DM-8647 ) so it's no longer needed. I believe I adopted all your other suggested changes (except a few that were questions "is this really needed?" to which the answer was "yes").

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              fred3m Fred Moolekamp
              Reviewers:
              Krzysztof Findeisen
              Watchers:
              Fred Moolekamp, Krzysztof Findeisen, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.