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

Improve Python wrapping of lsst::afw::image::detail

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: To Do
    • Resolution: Unresolved
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None

      Description

      lsst::afw::image::detail contains 4 functions plus a few typedefs and such that are not relevant to python. SWIG wrapped those functions in namespace lsst.afw.image, which made them look like part of our API. This was likely done for convenience, since it is extra work to add another namespace for wrapped code.

      Of the 4 functions, one is presently being used by jointcal (a usage which will be easier to eliminate once bbox is part of ExposureInfo), one is not used and 2 or 3 have unit tests.

      For now we have emulated that in pybind11, e.g. in DM-7801. However, we should go back and improve this. My proposal is to emulate the C++ and expose the functions in lsst.afw.image.detail (in which case one would have to explicitly import that namespace to use it). This has the advantage of being easiest to explain to users while still trivially allowing the functions to be unit tested. Others have proposed hiding the symbols more deeply, e.g. by moving the wrappers to the test directory, so that the functions cannot be used anywhere else. Another option is to wrap using a leading underscore (with or without moving it down to the detail sub-namespace). This would make the symbol much harder to get to, since it would not be brought in by from imageLib import *. Again, though, I suggest we just emulate the C++. It's simple and clearly abuse has been quite limited even with no hints that the code is not part of the API.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Frustratingly, this issue doesn't actually say which four functions it's concerned about. After a bit of searching, I believe they are:

            • stripCalibKeywords
            • stripFilterKeywords
            • setVisitInfoMetadata
            • stripVisitInfoKeywords

            Of these, the first was removed (on DM-10156). The remaining three still exist, though, so I guess this issue is still valid.

            Show
            swinbank John Swinbank added a comment - Frustratingly, this issue doesn't actually say which four functions it's concerned about. After a bit of searching, I believe they are: stripCalibKeywords stripFilterKeywords setVisitInfoMetadata stripVisitInfoKeywords Of these, the first was removed (on DM-10156 ). The remaining three still exist, though, so I guess this issue is still valid.
            Hide
            price Paul Price added a comment - - edited

            I built the latter two functions into some PFS code just the other day.

            Show
            price Paul Price added a comment - - edited I built the latter two functions into some PFS code just the other day.

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              rowen Russell Owen
              Watchers:
              Fred Moolekamp, Jim Bosch, John Swinbank, Paul Price, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:

                  Jenkins

                  No builds found.