# 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

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.
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 .

#### People

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