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

Cleanup pybind11 remaining code

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      4
    • Sprint:
      DRP S17-4, DRP S17-5
    • Team:
      Data Release Production

      Description

      Use the checklist from DM-9182 to clean up code in all packages not covered by DM-9182 or DM-9188. Most work will probably be ip_diffim and meas_modelfit, as most other packages have relatively little wrapped C++.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            I still think they ought to go in the pybind11 files. But won't need nearly as many of them once we merge things into larger modules, because all of the intra-package dependencies will already be accounted for.

            Show
            jbosch Jim Bosch added a comment - I still think they ought to go in the pybind11 files. But won't need nearly as many of them once we merge things into larger modules, because all of the intra-package dependencies will already be accounted for.
            Hide
            rowen Russell Owen added a comment -

            My understanding is that this cleanup pass is an intermediate stage: the final stage will be to mostly combined the small wrappers into larger wrappers. That is stated as the justification for not having pybind11 import statements in the wrappers. But I am not very happy about this. I would much have this code be correct, with the imports in place so that the modules are self-contained, as we expect them to be. That way we can take our time about the final cleanup pass.

            But even if we do decide to go for a quick and dirty solution for imports, for now, something needs to be done because as far as I can see many imports packages are not being imported anywhere, not even the __init__.py files. This makes the code needlessly fragile.

            I'll put this back into In Progress.

            Show
            rowen Russell Owen added a comment - My understanding is that this cleanup pass is an intermediate stage: the final stage will be to mostly combined the small wrappers into larger wrappers. That is stated as the justification for not having pybind11 import statements in the wrappers. But I am not very happy about this. I would much have this code be correct, with the imports in place so that the modules are self-contained, as we expect them to be. That way we can take our time about the final cleanup pass. But even if we do decide to go for a quick and dirty solution for imports, for now, something needs to be done because as far as I can see many imports packages are not being imported anywhere, not even the __init__.py files. This makes the code needlessly fragile. I'll put this back into In Progress.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            I added py::module::import statements for all cross package imports that were explicitly needed. Furthermore I changed all from __future__ import lines to the standard as requested.
            Finally I verified the build on Jenkins and fixed some minor issues here and there.
            Ready for second review.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - I added py::module::import statements for all cross package imports that were explicitly needed. Furthermore I changed all from __future__ import lines to the standard as requested. Finally I verified the build on Jenkins and fixed some minor issues here and there. Ready for second review.
            Hide
            rowen Russell Owen added a comment -

            Overall this looks great. Many of the packages can be merged with no changes. However, there are still a few missing imports in some packages and I found one or two other things.

            Show
            rowen Russell Owen added a comment - Overall this looks great. Many of the packages can be merged with no changes. However, there are still a few missing imports in some packages and I found one or two other things.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Merged after implementing comments and CI.
            Thanks for the review!

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Merged after implementing comments and CI. Thanks for the review!

              People

              Assignee:
              pschella Pim Schellart [X] (Inactive)
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Russell Owen
              Watchers:
              Jim Bosch, Pim Schellart [X] (Inactive), Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: