Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-8264

Finish wrapping afw::table with pybind11

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw

      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

        Attachments

          Issue Links

            Activity

            Hide
            fred3m 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
            fred3m 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
            rowen 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
            rowen 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
            pschella 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
            pschella 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
            rowen 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
            rowen 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
            rowen 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
            rowen 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:
                rowen Russell Owen
                Reporter:
                fred3m Fred Moolekamp
                Reviewers:
                Fred Moolekamp
                Watchers:
                Fred Moolekamp, Pim Schellart [X] (Inactive), Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel