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

Wrap ip_diffim with pybind11

    Details

    • Story Points:
      5
    • Sprint:
      Alert Production S17 - 1
    • Team:
      Alert Production

      Attachments

        Issue Links

          Activity

          pschella Pim Schellart [X] (Inactive) created issue -
          pschella Pim Schellart [X] (Inactive) made changes -
          Field Original Value New Value
          Epic Link DM-7717 [ 26925 ]
          swinbank John Swinbank made changes -
          Labels SciencePipelines
          krzys Krzysztof Findeisen made changes -
          Link This issue blocks DM-8461 [ DM-8461 ]
          krzys Krzysztof Findeisen made changes -
          Link This issue is blocked by DM-8453 [ DM-8453 ]
          krzys Krzysztof Findeisen made changes -
          Link This issue is blocked by DM-8419 [ DM-8419 ]
          krzys Krzysztof Findeisen made changes -
          Link This issue is blocked by DM-8457 [ DM-8457 ]
          pschella Pim Schellart [X] (Inactive) made changes -
          Assignee Russell Owen [ rowen ]
          rowen Russell Owen made changes -
          Story Points 4 8
          rowen Russell Owen made changes -
          Sprint Alert Production S17 - 1 [ 355 ]
          Team Alert Production [ 10300 ]
          rowen Russell Owen made changes -
          Status To Do [ 10001 ] In Progress [ 3 ]
          rowen Russell Owen made changes -
          Story Points 8 5
          Hide
          rowen Russell Owen added a comment -

          This consists of several parts:

          • Wrap the code with pybind11
          • Rework the C++ to not use shared_ptr to Eigen matrices or vectors (it's not wanted and pybind11 had problems with at least one instance). I used const & where I could and passed by value everywhere else.
          • Modernize python imports: from __future__ import division was needed in some places in order for python 2 unit tests to pass, and standardized the grouping and fixed a few minor flake8 warnings (I left a few lines that were too long because they would be hard to fix)
          Show
          rowen Russell Owen added a comment - This consists of several parts: Wrap the code with pybind11 Rework the C++ to not use shared_ptr to Eigen matrices or vectors (it's not wanted and pybind11 had problems with at least one instance). I used const & where I could and passed by value everywhere else. Modernize python imports: from __future__ import division was needed in some places in order for python 2 unit tests to pass, and standardized the grouping and fixed a few minor flake8 warnings (I left a few lines that were too long because they would be hard to fix)
          rowen Russell Owen made changes -
          Reviewers Fred Moolekamp [ fred3m ]
          Status In Progress [ 3 ] In Review [ 10004 ]
          Hide
          fred3m Fred Moolekamp added a comment -

          Everything looks good, I just had a few very minor comments. I am a little confused about what did and didn't cause trouble with Eigen Matrices. The ticket makes it sound like you had to modify the C++ code to remove shared_ptr's to Eigen Matrices but I didn't see where those modifications were made. Could you elaborate a little more on what you did, and what files were modified?

          It would also be nice to run clang-format in a separate commit before you touch a file. It might be a pain to have to run it twice, once before editing the file and once after, but it was very difficult to follow the changes you made vs. the changes made by clang format, so I had to review all of the old code as well. Editing a file and running clang-format after you've modified the file can (and should) be included in the same commit.

          Show
          fred3m Fred Moolekamp added a comment - Everything looks good, I just had a few very minor comments. I am a little confused about what did and didn't cause trouble with Eigen Matrices. The ticket makes it sound like you had to modify the C++ code to remove shared_ptr's to Eigen Matrices but I didn't see where those modifications were made. Could you elaborate a little more on what you did, and what files were modified? It would also be nice to run clang-format in a separate commit before you touch a file. It might be a pain to have to run it twice, once before editing the file and once after, but it was very difficult to follow the changes you made vs. the changes made by clang format, so I had to review all of the old code as well. Editing a file and running clang-format after you've modified the file can (and should) be included in the same commit.
          fred3m Fred Moolekamp made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          rowen Russell Owen made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]
          tjenness Tim Jenness made changes -
          Component/s ip_diffim [ 10743 ]

            People

            • Assignee:
              rowen Russell Owen
              Reporter:
              pschella Pim Schellart [X] (Inactive)
              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