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
(1 relates to)
Activity
| Description |
The way {{BaseRecord}} is wrapped in pybind11 needs some work:
- {{get}} and {{\_\_getitem\_\_}} should be identical (as they were with SWIG) but they are different and neither one can be used in place of the other. One or two tests fail either way. Furthermore the code bears little resemblance to the original SWIG (for good reasons, but I understand Fred knows how to work around this now). - The wrappings should probably use C++ method {{_get}}, which needs to be added in the pybind11 wrapper file. - The C++ wrapper defines {{_getitem_}} for each data type and the code is radically different than for {{_get_ + suffix}}; I hope it can be eliminated or made the same as {{_get_ + suffix}} and renamed to {{_get}}. |
The way {{BaseRecord}} is wrapped in pybind11 needs some work:
- {{get}} and {{\_\_getitem\_\_}} should be wrapped identically (as they were with SWIG) or should actually be 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}}; I hope it can be eliminated or made the same as {{_get_ + suffix}} and renamed to {{_get}}. |
| Description |
The way {{BaseRecord}} is wrapped in pybind11 needs some work:
- {{get}} and {{\_\_getitem\_\_}} should be wrapped identically (as they were with SWIG) or should actually be 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}}; I hope it can be eliminated or made the same as {{_get_ + suffix}} and renamed to {{_get}}. |
The way {{BaseRecord}} is wrapped in pybind11 needs some work:
- {{get}} and {{\_\_getitem\_\_}} should be wrapped identically (as they were with SWIG) or should actually be 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. |
| Description |
The way {{BaseRecord}} is wrapped in pybind11 needs some work:
- {{get}} and {{\_\_getitem\_\_}} should be wrapped identically (as they were with SWIG) or should actually be 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. |
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. |
Fred Moolekamp
made changes -
| Assignee | Fred Moolekamp [ fred3m ] |
| Assignee | Jim Bosch [ jbosch ] |
| Epic Link |
|
| Epic Link |
|
| Epic Link |
|
| Team | Data Release Production [ 10301 ] |
In progress, but the remainder of the work has been deferred until we can run more integration-level tests on the pybind11 stack.
| Status | To Do [ 10001 ] | In Progress [ 3 ] |
| Summary | BaseRecord wrapping of set and get needs work | Use visitor pattern to clean up afw::table pybind11 wrappers |
| 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. |
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. |
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. | Sprint | DRP S17-3 [ 360 ] |
| Story Points | 6 |
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.
| Component/s | afw [ 10714 ] |
| Sprint | DRP S17-3 [ 360 ] | DRP S17-3, DRP S17-4 [ 360, 363 ] |
| Rank | Ranked higher |
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.
| Reviewers | Krzysztof Findeisen, Pim Schellart, Russell Owen [ krzys, pschella, rowen ] | |
| Status | In Progress [ 3 ] | In Review [ 10004 ] |
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. Krzysztof Findeisen
made changes -
| Reviewers | Krzysztof Findeisen, Pim Schellart, Russell Owen [ krzys, pschella, rowen ] | Pim Schellart, Russell Owen [ pschella, rowen ] |
Looks fine.
Pim Schellart [X] (Inactive)
added a comment - Looks fine. Pim Schellart [X] (Inactive)
made changes -
| Status | In Review [ 10004 ] | Reviewed [ 10101 ] |
| Resolution | Done [ 10000 ] | |
| Status | Reviewed [ 10101 ] | Done [ 10002 ] |
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.