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

Wrap meas_astrom with pybind11

    XMLWordPrintable

    Details

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

      Attachments

        Issue Links

          Activity

          No builds found.
          pschella Pim Schellart [X] (Inactive) created issue -
          pschella Pim Schellart [X] (Inactive) made changes -
          Field Original Value New Value
          Epic Link DM-7717 [ 26925 ]
          swinbank John Swinbank made changes -
          Labels SciencePipelines
          krzys Krzysztof Findeisen made changes -
          Link This issue blocks DM-8461 [ DM-8461 ]
          krzys Krzysztof Findeisen made changes -
          Link This issue is blocked by DM-8453 [ DM-8453 ]
          krzys Krzysztof Findeisen made changes -
          Link This issue blocks DM-8463 [ DM-8463 ]
          krzys Krzysztof Findeisen made changes -
          Link This issue blocks DM-8466 [ DM-8466 ]
          rowen Russell Owen made changes -
          Assignee Russell Owen [ rowen ]
          rowen Russell Owen made changes -
          Status To Do [ 10001 ] In Progress [ 3 ]
          rowen Russell Owen made changes -
          Sprint Alert Production S17 - 12 [ 305 ]
          Story Points 2.4 5
          Team Alert Production [ 10300 ]
          rowen Russell Owen made changes -
          Link This issue is blocked by DM-8453 [ DM-8453 ]
          krughoff Simon Krughoff made changes -
          Sprint Alert Production S17 - 12 [ 305 ] Alert Production S17 - 12, Alert Production S17 - 1 [ 305, 355 ]
          krughoff Simon Krughoff made changes -
          Rank Ranked higher
          rowen Russell Owen made changes -
          Link This issue is blocked by DM-8453 [ DM-8453 ]
          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.
          rowen Russell Owen made changes -
          Reviewers Pim Schellart [ pschella ]
          Status In Progress [ 3 ] In Review [ 10004 ]
          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.
          pschella Pim Schellart [X] (Inactive) made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          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.
          rowen Russell Owen made changes -
          Status Reviewed [ 10101 ] In Review [ 10004 ]
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Looks good.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Looks good.
          rowen Russell Owen made changes -
          Story Points 5 6
          rowen Russell Owen made changes -
          Resolution Done [ 10000 ]
          Status In Review [ 10004 ] Done [ 10002 ]
          Parejkoj John Parejko made changes -
          Link This issue blocks DM-9187 [ DM-9187 ]
          tjenness Tim Jenness made changes -
          Component/s meas_astrom [ 10745 ]

            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:

                CI Builds

                No builds found.