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

Wrap display_ds9 with pybind11

    XMLWordPrintable

    Details

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

      Description

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

        Attachments

          Activity

          Hide
          rowen Russell Owen added a comment -

          It has SWIGged code in it, so definitely needs work. Unfortunately it also has no tests.

          Show
          rowen Russell Owen added a comment - It has SWIGged code in it, so definitely needs work. Unfortunately it also has no tests.
          Hide
          rowen Russell Owen added a comment - - edited

          The changes are:

          display_ds9:

          • wrap a custom C++ interface to xpa. I defined this in the same file as the wrapper because that is how the SWIG did it, there was no header file and it uses globals. I felt safer duplicating what SWIG did. Even so I had to tweak the code a bit because SWIG has the ability to raise a useful error if a function returns NULL and pybind11 does not, as far as I know). So instead of returning NULL I raise an exception.
          • ds9.py function _i_mtv has special handling for uint16 data and the check for that required a small update to work with pybind11.

          afw:

          • I wrapped the writeFitsImage function. I believe this completes all wrapping of afw/display.

          I tested it by running the following snippet:

          import os.path
           
          from lsst.utils import getPackageDir
          import lsst.afw.display as afwDisplay
          import lsst.afw.image as afwImage
          import lsst.afw.geom as afwGeom
          import lsst.display.ds9
           
          afwDataDir = getPackageDir("afwdata")
          assert afwDataDir
           
          path = os.path.join(afwDataDir, "ImSim/calexp/v85408556-fr/R23/S11.fits")
          subregion = afwGeom.Box2I(afwGeom.Point2I(0, 0), afwGeom.Extent2I(500, 500))
          exp = afwImage.ExposureF(path, subregion)
          mi = exp.getMaskedImage()
          image = mi.getImage()
          mask = mi.getMask()
           
          disp = afwDisplay.getDisplay(1)
          disp.mtv(exp, title="foo")
          

          and also modifying the snippet to only show an image or a mask.

          Show
          rowen Russell Owen added a comment - - edited The changes are: display_ds9: wrap a custom C++ interface to xpa . I defined this in the same file as the wrapper because that is how the SWIG did it, there was no header file and it uses globals. I felt safer duplicating what SWIG did. Even so I had to tweak the code a bit because SWIG has the ability to raise a useful error if a function returns NULL and pybind11 does not, as far as I know). So instead of returning NULL I raise an exception. ds9.py function _i_mtv has special handling for uint16 data and the check for that required a small update to work with pybind11. afw: I wrapped the writeFitsImage function. I believe this completes all wrapping of afw/display . I tested it by running the following snippet: import os.path   from lsst.utils import getPackageDir import lsst.afw.display as afwDisplay import lsst.afw.image as afwImage import lsst.afw.geom as afwGeom import lsst.display.ds9   afwDataDir = getPackageDir("afwdata") assert afwDataDir   path = os.path.join(afwDataDir, "ImSim/calexp/v85408556-fr/R23/S11.fits") subregion = afwGeom.Box2I(afwGeom.Point2I(0, 0), afwGeom.Extent2I(500, 500)) exp = afwImage.ExposureF(path, subregion) mi = exp.getMaskedImage() image = mi.getImage() mask = mi.getMask()   disp = afwDisplay.getDisplay(1) disp.mtv(exp, title="foo") and also modifying the snippet to only show an image or a mask.
          Hide
          krzys Krzysztof Findeisen added a comment -

          Sigh I can see why you were afraid to touch the Swig code. Do you know if there is a technical debt ticket for cleaning up this package?

          Aside from the contents of _xpa.cc/xpa.i, my only requests are to make the module structure a bit more pybind11-standard and to clean up the commit history.

          Show
          krzys Krzysztof Findeisen added a comment - Sigh I can see why you were afraid to touch the Swig code. Do you know if there is a technical debt ticket for cleaning up this package? Aside from the contents of _xpa.cc / xpa.i , my only requests are to make the module structure a bit more pybind11-standard and to clean up the commit history.
          Hide
          rowen Russell Owen added a comment - - edited

          Thank you for the helpful (and quick!) review. I fixed the issues you caught, including rebasing to correct the contents of one commit and fixing .gitignore.

          I filed DM-9273 for the technical debt in display_ds9's xpa wrapper. I also added xpa.py which imports lsst.pex.exceptions and the symbols defined in _xpa.cc. I chose not to add a displayLib.py because in this case only the xpa interface is wrapped and failure to import that suggests that 3rd party package xpa is missing or broken. So in this case I feel it is preferable for that one module to be self-contained.

          Show
          rowen Russell Owen added a comment - - edited Thank you for the helpful (and quick!) review. I fixed the issues you caught, including rebasing to correct the contents of one commit and fixing .gitignore . I filed DM-9273 for the technical debt in display_ds9 's xpa wrapper. I also added xpa.py which imports lsst.pex.exceptions and the symbols defined in _xpa.cc . I chose not to add a displayLib.py because in this case only the xpa interface is wrapped and failure to import that suggests that 3rd party package xpa is missing or broken. So in this case I feel it is preferable for that one module to be self-contained.

            People

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

              Dates

              Created:
              Updated:
              Resolved:

                CI Builds

                No builds found.