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

Wrap utils with pybind11

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: utils
    • Labels:
      None
    • Story Points:
      2
    • Sprint:
      DRP F16-2, DRP F16-3
    • Team:
      Data Release Production

      Description

      Split off from DM-6302.

        Attachments

          Activity

          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Hmm, not sure what the right approach is here.
          There seems to be a lot of very Swig specific stuff in this package (e.g typemaps for numpy types and Swig macros for operators and function return types, as well as some workarounds for Swig problems).
          Some of it looks like we might need a pybind11 equivalent for, but we probably don't need everything and we won't quite know what we need until we encounter it.
          I see three options.

          1. Remove all Swig specific stuff and add things back in (either here or elsewhere) as needed when wrapping further packages.
          2. Create as much as possible equivalent functionality now, at the risk of not actually needing it (or needing a different version of it) later.
          3. Postpone work on this ticket until we know what we need.

          Either way some of the tests also need to be removed (since it makes no sense to test the Swig specific things). So it will be hard to check if the conversion is complete.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Hmm, not sure what the right approach is here. There seems to be a lot of very Swig specific stuff in this package (e.g typemaps for numpy types and Swig macros for operators and function return types, as well as some workarounds for Swig problems). Some of it looks like we might need a pybind11 equivalent for, but we probably don't need everything and we won't quite know what we need until we encounter it. I see three options. 1. Remove all Swig specific stuff and add things back in (either here or elsewhere) as needed when wrapping further packages. 2. Create as much as possible equivalent functionality now, at the risk of not actually needing it (or needing a different version of it) later. 3. Postpone work on this ticket until we know what we need. Either way some of the tests also need to be removed (since it makes no sense to test the Swig specific things). So it will be hard to check if the conversion is complete.
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Note that this will be merged to the epic branch instead of master.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Note that this will be merged to the epic branch instead of master.
          Hide
          tjenness Tim Jenness added a comment -

          Looks great. I love the simplicity of the pybind11 wrapper files. Minor comments on the PR, probably down to me being late to this topic and being confused.

          Show
          tjenness Tim Jenness added a comment - Looks great. I love the simplicity of the pybind11 wrapper files. Minor comments on the PR, probably down to me being late to this topic and being confused.
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Merged into epic branch after incorporating requested changes.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Merged into epic branch after incorporating requested changes.

            People

            Assignee:
            pschella Pim Schellart [X] (Inactive)
            Reporter:
            pschella Pim Schellart [X] (Inactive)
            Reviewers:
            Tim Jenness
            Watchers:
            Jim Bosch, John Swinbank, Pim Schellart [X] (Inactive), Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.