Details
-
Type:
Improvement
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw
-
Labels:
-
Story Points:6
-
Epic Link:
-
Sprint:DRP S17-3, DRP S17-4
-
Team:Data Release Production
Description
The way BaseRecord is wrapped in pybind11 needs some work:
- get and __getitem__ should be wrapped the way they were with SWIG, which is simpler code than was used, and looks identical for the two methods (though that code uses $action so there are subtle differences). Better yet, make the methods identical if possible.
- The wrappings should probably use C++ method _get, which needs to be added in the pybind11 wrapper file, rather than the specializations that use suffixes.
- The C++ wrapper defines _getitem_ for each data type and the code is radically different than for _get_ + suffix. It would be nice if the differences could be eliminated.
Similar issues apply to several other afw::table classes.
Attachments
Issue Links
- blocks
-
DM-9100 Merge pybind11 branches to master
- Done
- is blocked by
-
- Done
-
- Done
- relates to
-
- Done
-
- Done
-
- Invalid
Activity
In progress, but the remainder of the work has been deferred until we can run more integration-level tests on the pybind11 stack.
pipe_tasks uses explicit calls to getL, getI, getD, and getFlag. The resolution of this issue may affect the validity (or lack thereof) of such calls.
Krzysztof Findeisen
added a comment - - edited pipe_tasks uses explicit calls to getL , getI , getD , and getFlag . The resolution of this issue may affect the validity (or lack thereof) of such calls. Please also change PeakCatalog to be a SortedCatalog (since it has an ID field) or perhaps explain in the .h file why you chose not to do so.
Finally ready for review. Since it's a weekend, I'm not expecting anyone to have time to look at it before we merge on Monday, but if you'd like to take a look, go ahead! I imagine on Monday morning we'll probably merge tickets like this first and then do a post-merge cleanup ticket to retroactively address any review comments.
I'm done with my review (since there are two other reviewers I will not mark it reviewed). Overall this looks great to me; a big improvement. I put some comments on github and wish I had more time to understand some of the changes, but don't want to hold up the merge.
I'm done with my review. I found some style issues and a few things I couldn't understand, but I agree that we shouldn't hold up the merge.
Krzysztof Findeisen
added a comment - I'm done with my review. I found some style issues and a few things I couldn't understand, but I agree that we shouldn't hold up the merge. Looks fine.
Pim Schellart [X] (Inactive)
added a comment - Looks fine.
While get and __getitem__ have a similar structure in the SWIG code, the $action command is different.
__getitem__ calls the C++ operator[] method and returns a reference, while getX returns a value, and specifies the type that it expects. For convenience (and to make the code more pythonic) get was implemented in SWIG so that the user doesn't need to specify the type. However, there are numerous tests and places in the code that use getX methods, so when I wrapped afw::table I had to keep that design. This causes the python code to look slightly different since pybind11 doesn't have a $action command.
So completing this ticket would require changing the API for afw::table, which is probably not a good idea until after the pybind11 port is successful.