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

Finish wrapping afw::table with pybind11

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • 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

            No builds found.
            fred3m Fred Moolekamp created issue -
            fred3m Fred Moolekamp made changes -
            Field Original Value New Value
            Epic Link DM-7717 [ 26925 ]
            rowen Russell Owen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            rowen Russell Owen made changes -
            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
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8363 [ DM-8363 ]
            pschella Pim Schellart [X] (Inactive) made changes -
            Link This issue blocks DM-8417 [ DM-8417 ]
            rowen Russell Owen made changes -
            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
            rowen Russell Owen made changes -
            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
            rowen Russell Owen made changes -
            Link This issue relates to DM-8415 [ DM-8415 ]
            rowen Russell Owen made changes -
            Reviewers Fred Moolekamp [ fred3m ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            rowen 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.

            rowen 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 .
            rowen Russell Owen made changes -
            Sprint Alert Production F16 - 11c [ 296 ]
            Labels SciencePipelines
            rowen 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.

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

            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.

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

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

            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.

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

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

            pschella Pim Schellart [X] (Inactive) added a comment - Having fewer commits also makes continuous rebasing on master way easier.
            rowen Russell Owen made changes -
            Link This issue relates to DM-6168 [ DM-6168 ]
            rowen Russell Owen made changes -
            Epic Link DM-7717 [ 26925 ] DM-8450 [ 28066 ]

            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.

            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.
            fred3m Fred Moolekamp made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            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.

            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.

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

            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 ).
            rowen Russell Owen added a comment -

            pschella thank you for the explanation. fred3m my apologies for making the PR with the wrong base.

            rowen Russell Owen added a comment - pschella thank you for the explanation. fred3m my apologies for making the PR with the wrong base.
            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.

            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 .
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            tjenness Tim Jenness made changes -
            Component/s afw [ 10714 ]

            People

              rowen Russell Owen
              fred3m Fred Moolekamp
              Fred Moolekamp
              Fred Moolekamp, Pim Schellart [X] (Inactive), Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.