# Wrap meas_astrom with pybind11

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
6
• Sprint:
• Team:

#### Activity

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

Show
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.
 Status Reviewed [ 10101 ] In Review [ 10004 ]
Hide
Pim Schellart [X] (Inactive) added a comment -

Looks good.

Show
Pim Schellart [X] (Inactive) added a comment - Looks good.
 Story Points 5 6
 Resolution Done [ 10000 ] Status In Review [ 10004 ] Done [ 10002 ]
 Link This issue blocks DM-9187 [ DM-9187 ]
 Component/s meas_astrom [ 10745 ]

#### People

Assignee:
Russell Owen
Reporter:
Pim Schellart [X] (Inactive)
Reviewers:
Pim Schellart [X] (Inactive)
Watchers:
Krzysztof Findeisen, Pim Schellart [X] (Inactive), Russell Owen