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

Wrap meas_astrom with pybind11

    Details

    • Story Points:
      6
    • Sprint:
      Alert Production S17 - 12, Alert Production S17 - 1
    • Team:
      Alert Production

      Attachments

        Issue Links

          Activity

          Hide
          rowen Russell Owen added a comment -

          I had to wrap LinearTransform.operator() in afw.
          Otherwise it was straightforward.

          However, I am not very happy with astrometry_net.cc. It has a lot of C++ code mixed in with the usual pybind11 wrapper. The extra code is there as a shim to astrometry_net's C code, so it's reasonable to have there, but the file feels cluttered to me. Suggestions for cleanup welcome.

          Note that we will be moving our wrapper of astrometry_net into a new package at some point (soon after we switch to pybind11, I hope, but so far other things have always taken priority). So if a very invasive sort of cleanup is wanted, it might be best to defer that until the split.

          Show
          rowen Russell Owen added a comment - I had to wrap LinearTransform.operator() in afw. Otherwise it was straightforward. However, I am not very happy with astrometry_net.cc . It has a lot of C++ code mixed in with the usual pybind11 wrapper. The extra code is there as a shim to astrometry_net's C code, so it's reasonable to have there, but the file feels cluttered to me. Suggestions for cleanup welcome. Note that we will be moving our wrapper of astrometry_net into a new package at some point (soon after we switch to pybind11, I hope, but so far other things have always taken priority). So if a very invasive sort of cleanup is wanted, it might be best to defer that until the split.
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Looks good.

          I would however prefer to split the C++ shim from the wrapper code.
          Feel free to disagree, or choose between a single `astronomy_net.h + .cc` combo (in the standard `include` / `src` locations) or having separate `MultiIndex.h` + `Solver.h` (+ `.cc`) files.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Looks good. I would however prefer to split the C++ shim from the wrapper code. Feel free to disagree, or choose between a single `astronomy_net.h + .cc` combo (in the standard `include` / `src` locations) or having separate `MultiIndex.h` + `Solver.h` (+ `.cc`) files.
          Hide
          rowen Russell Owen added a comment - - edited

          I split the file as you suggested and added MultiIndex.__iter__ in Python. Note that I left the logging functions in the pybind11 wrapper because I think they would need more work to be made visible to C++ code (more than I think is justified at this time).

          Please have another look.

          Show
          rowen Russell Owen added a comment - - edited I split the file as you suggested and added MultiIndex.__iter__ in Python. Note that I left the logging functions in the pybind11 wrapper because I think they would need more work to be made visible to C++ code (more than I think is justified at this time). Please have another look.
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Looks good.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Looks good.

            People

            • Assignee:
              rowen Russell Owen
              Reporter:
              pschella Pim Schellart [X] (Inactive)
              Reviewers:
              Pim Schellart [X] (Inactive)
              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: