# Finish wrapping afw::table with pybind11

XMLWordPrintable

#### Details

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

#### Description

Learn how to wrap code using pybind11 and wrap afw::table tests not wrapped in DM-7056. These tests are

1. testTableAliases.py
2. testTicket2026.py
3. testTicket2707.py
4. testMatchFits.py
5. testSourceMatch.py # except a pickle segfault to be fixed in DM-8415
6. testAmpInfoTable.py
7. testAstropyTableViews.py

#### Activity

No builds found.
Fred Moolekamp created issue -
Field Original Value New Value
Epic Link DM-7717 [ 26925 ]
 Status To Do [ 10001 ] In Progress [ 3 ]
 Description Learn how to wrap code using pybind11 and wrap afw::table tests not wrapped in DM-7056. These tests are # testTableAliases.py # ticket2026.py # ticket2707.py # matchFits.py # sourceMatch.py # testAmpInfoTable.py # testAstropyTableViews.py Learn how to wrap code using pybind11 and wrap afw::table tests not wrapped in DM-7056. These tests are # testTableAliases.py # testTicket2026.py # testTicket2707.py # testMatchFits.py # testSourceMatch.py # testAmpInfoTable.py # testAstropyTableViews.py
 Link This issue is blocked by DM-8363 [ DM-8363 ]
Pim Schellart [X] (Inactive) made changes -
 Link This issue blocks DM-8417 [ DM-8417 ]
 Description Learn how to wrap code using pybind11 and wrap afw::table tests not wrapped in DM-7056. These tests are # testTableAliases.py # testTicket2026.py # testTicket2707.py # testMatchFits.py # testSourceMatch.py # testAmpInfoTable.py # testAstropyTableViews.py Learn how to wrap code using pybind11 and wrap afw::table tests not wrapped in DM-7056. These tests are # testTableAliases.py # testTicket2026.py # testTicket2707.py # testMatchFits.py # testSourceMatch.py # except a pickle segfault to be fixed in DM-8417 # testAmpInfoTable.py # testAstropyTableViews.py
 Description Learn how to wrap code using pybind11 and wrap afw::table tests not wrapped in DM-7056. These tests are # testTableAliases.py # testTicket2026.py # testTicket2707.py # testMatchFits.py # testSourceMatch.py # except a pickle segfault to be fixed in DM-8417 # testAmpInfoTable.py # testAstropyTableViews.py Learn how to wrap code using pybind11 and wrap afw::table tests not wrapped in DM-7056. These tests are # testTableAliases.py # testTicket2026.py # testTicket2707.py # testMatchFits.py # testSourceMatch.py # except a pickle segfault to be fixed in DM-8415 # testAmpInfoTable.py # testAstropyTableViews.py
 Link This issue relates to DM-8415 [ DM-8415 ]
 Reviewers Fred Moolekamp [ fred3m ] Status In Progress [ 3 ] In Review [ 10004 ]
Hide
Russell Owen added a comment - - edited

I refactored how tables are wrapped, so various kinds of tables can use a few central functions, including declareCatalog and declareSortedCatalog (C++) and addCatalogMethods (Python).

I also tweaked the C++ formatting to match our standards or common usage, including changing
using namespace lsst::afw::table; to namespace lsst { namespace afw { namespace table
and const std::string & to std::string const &.

All the tests pass except testSourceMatch.py, which segfaults unless pickling is prevented, e.g. by adding a return at the beginning of checkPickle. Pickling will be fixed in DM-8415.

Show
Russell Owen added a comment - - edited I refactored how tables are wrapped, so various kinds of tables can use a few central functions, including declareCatalog and declareSortedCatalog (C++) and addCatalogMethods (Python). I also tweaked the C++ formatting to match our standards or common usage, including changing using namespace lsst::afw::table; to namespace lsst { namespace afw { namespace table and const std::string & to std::string const &. All the tests pass except testSourceMatch.py, which segfaults unless pickling is prevented, e.g. by adding a return at the beginning of checkPickle. Pickling will be fixed in DM-8415 .
 Sprint Alert Production F16 - 11c [ 296 ] Labels SciencePipelines
Hide
Russell Owen added a comment -

I am open to suggestions on how to better group the commits. I tried to group related changes but it feels a bit messy.

Show
Russell Owen added a comment - I am open to suggestions on how to better group the commits. I tried to group related changes but it feels a bit messy.
Hide
Pim Schellart [X] (Inactive) added a comment -

I think we should really squash most commits together into a single "Wrap afw::table with pybind11" commit on DM-6168. Only when things get changed in the library code itself (i.e. not the wrappers or tests thereof) should these commits be kept separate. Otherwise the final merge to master will introduce far too many pointless commits.

Show
Pim Schellart [X] (Inactive) added a comment - I think we should really squash most commits together into a single "Wrap afw::table with pybind11" commit on DM-6168 . Only when things get changed in the library code itself (i.e. not the wrappers or tests thereof) should these commits be kept separate. Otherwise the final merge to master will introduce far too many pointless commits.
Hide
Russell Owen added a comment -

That makes sense. What about adding new library code that is only intended for pybind11 wrapping (as this ticket does)? I'm hoping it can all be part of the same commit, since it'll be messy to tease that out otherwise.

Show
Russell Owen added a comment - That makes sense. What about adding new library code that is only intended for pybind11 wrapping (as this ticket does)? I'm hoping it can all be part of the same commit, since it'll be messy to tease that out otherwise.
Hide
Pim Schellart [X] (Inactive) added a comment -

Yes, I would say so. In the end just having one commit per package for the wrapping makes most sense. Only in the case of afw, because it is so big, might it make sense to split it up per component namespace.

Show
Pim Schellart [X] (Inactive) added a comment - Yes, I would say so. In the end just having one commit per package for the wrapping makes most sense. Only in the case of afw , because it is so big, might it make sense to split it up per component namespace.
Hide
Pim Schellart [X] (Inactive) added a comment -

Having fewer commits also makes continuous rebasing on master way easier.

Show
Pim Schellart [X] (Inactive) added a comment - Having fewer commits also makes continuous rebasing on master way easier.
 Link This issue relates to DM-6168 [ DM-6168 ]
 Epic Link DM-7717 [ 26925 ] DM-8450 [ 28066 ]
Hide
Fred Moolekamp added a comment -

A had few minor comments on the ticket and a suggestion about the structure based on a conversation with Pim. In general I think it is a good idea to stick with the convention that there should be a pybind11 .cc file for each .h file in the include directory tree, but in this case I believe an exception was warranted and that it was a good change. It is much easier to understand where SimpleCatalog, BaseCatalog, etc are defined using your new structure.

I'm not convinced that switching to the namespace lsst { ... usage is correct in this case. My understanding is that there is a difference between using and namespace is that the current method adds the declare functions to the namespace, whereas the older method did not. I like how you added a _all_ definition to the python modules to keep the namespace clean but I feel like this does the opposite. Perhaps I'm misunderstanding what it's doing but I think for this reason and for the consistency of the pybind11 code it might be best to switch these back.

This also needs to be rebased with DM-6168. The commit history of the pull request should only show the commits that you made, not the past commits. It's possible that DMTN-026 needs to be updated since Pim realized the correct procedure to rebase properly. It should be

 checkout tickets/DM-6168 git fetch git reset --hard origin/tickets/DM-6168 git checkout tickets/DM-8264 git rebase --onto tickets/DM-6168 a003834~ tickets/DM-8264 

where a003834 is the first commit you want to place on top of the rebased "master", DM-6168.

And make sure that it passes the tests in python 2 and 3 in Jenkins. It looks like you will need to include DM-7797 in your Jenkins build (there were some tests commented in that ticket that have not made their way to DM-6168 yet, which currently does not rebuild) or comment them yourself in this ticket.

Show
Fred Moolekamp added a comment - A had few minor comments on the ticket and a suggestion about the structure based on a conversation with Pim. In general I think it is a good idea to stick with the convention that there should be a pybind11 .cc file for each .h file in the include directory tree, but in this case I believe an exception was warranted and that it was a good change. It is much easier to understand where SimpleCatalog, BaseCatalog, etc are defined using your new structure. I'm not convinced that switching to the namespace lsst { ... usage is correct in this case. My understanding is that there is a difference between using and namespace is that the current method adds the declare functions to the namespace, whereas the older method did not. I like how you added a _ all _ definition to the python modules to keep the namespace clean but I feel like this does the opposite. Perhaps I'm misunderstanding what it's doing but I think for this reason and for the consistency of the pybind11 code it might be best to switch these back. This also needs to be rebased with DM-6168 . The commit history of the pull request should only show the commits that you made, not the past commits. It's possible that DMTN-026 needs to be updated since Pim realized the correct procedure to rebase properly. It should be checkout tickets /DM-6168 git fetch git reset --hard origin /tickets/DM-6168 git checkout tickets /DM-8264 git rebase --onto tickets /DM-6168 a003834~ tickets /DM-8264 where a003834 is the first commit you want to place on top of the rebased "master", DM-6168 . And make sure that it passes the tests in python 2 and 3 in Jenkins. It looks like you will need to include DM-7797 in your Jenkins build (there were some tests commented in that ticket that have not made their way to DM-6168 yet, which currently does not rebuild) or comment them yourself in this ticket.
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide
Russell Owen added a comment - - edited

DM-8264 is already rebased to DM-6168 (and I have done so several times during development) and I planned to do so again just before merging back to DM-6168, if necessary. Are you saying that I did this incorrectly? It looks right to me in SourceTree.

Show
Russell Owen added a comment - - edited DM-8264 is already rebased to DM-6168 (and I have done so several times during development) and I planned to do so again just before merging back to DM-6168 , if necessary. Are you saying that I did this incorrectly? It looks right to me in SourceTree.
Hide
Pim Schellart [X] (Inactive) added a comment -

No, you did everything correctly. The PR just had the diff against the wrong base (master instead of DM-6168).

Show
Pim Schellart [X] (Inactive) added a comment - No, you did everything correctly. The PR just had the diff against the wrong base (master instead of DM-6168 ).
Hide
Russell Owen added a comment -

Pim Schellart [X] thank you for the explanation. Fred Moolekamp my apologies for making the PR with the wrong base.

Show
Russell Owen added a comment - Pim Schellart [X] thank you for the explanation. Fred Moolekamp my apologies for making the PR with the wrong base.
Hide
Russell Owen added a comment -

Thank you for the helpful review.

I left the commented-out methods alone because I found it so useful when other methods I needed were already available but commented out. We will have to purge them at some point.

I also retained our standard usage of namespace lsst { namespace afw { namespace table and hope we will switch to that for all future pybind11 code in order to match all our other C++ code.

Other than that I made all requested changes.

Tested with Jenkins using Python 2 and Python 3 and merged to DM-6168.

Show
Russell Owen added a comment - Thank you for the helpful review. I left the commented-out methods alone because I found it so useful when other methods I needed were already available but commented out. We will have to purge them at some point. I also retained our standard usage of namespace lsst { namespace afw { namespace table and hope we will switch to that for all future pybind11 code in order to match all our other C++ code. Other than that I made all requested changes. Tested with Jenkins using Python 2 and Python 3 and merged to DM-6168 .
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]
 Component/s afw [ 10714 ]

#### People

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