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

Wrap example C++ code with Cython

    XMLWordPrintable

    Details

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

      Description

      Take the example C++ codebase developed in DM-5470, and expose it to Python in the most idiomatic possible way using Cython. Produce a technical note describing how this was carried out and discussing any particular pain points either in implementation or results.

        Attachments

          Issue Links

            Activity

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

            Ok, cleanup complete.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Ok, cleanup complete.
            Hide
            jbosch Jim Bosch added a comment -

            Review complete. I've made a few comments on the Cython python-cpp-challenge PR, as well as on some commits for the tech report (since I couldn't find a better way to make line comments on that).

            There are some minor grammatical issues, but I don't think it's worth either of our time for me to go through and point them all out to you, and they don't get in the way of understanding the document at all. If you don't mind, I'll just edit the tech report source myself to make those changes?

            The big comment I have is that I'd like to actually have some more "typical" wrapper code in the tech report at some level, with some annotation and a copy of the C++ code that it wraps - or at least enough that a reader can see what the Cython bindings for a typical method looks like.

            Show
            jbosch Jim Bosch added a comment - Review complete. I've made a few comments on the Cython python-cpp-challenge PR, as well as on some commits for the tech report (since I couldn't find a better way to make line comments on that). There are some minor grammatical issues, but I don't think it's worth either of our time for me to go through and point them all out to you, and they don't get in the way of understanding the document at all. If you don't mind, I'll just edit the tech report source myself to make those changes? The big comment I have is that I'd like to actually have some more "typical" wrapper code in the tech report at some level, with some annotation and a copy of the C++ code that it wraps - or at least enough that a reader can see what the Cython bindings for a typical method looks like.
            Hide
            jbosch Jim Bosch added a comment - - edited

            Also,

            I don't think I can fix the non-Pythonic nature of .as_list() or .as_dict because that would require storing a pointer to the Python objects in the DoodadSet as well, which kind of defeats the purpose of wrapping the container. But I might be wrong here.

            I think this is basically right. The closest you can get to the behavior you'd want in Python is to construct the C++ shared_ptr by putting a PyObject in the shared_ptr's deleter instead of putting a shared_ptr in the PyObject (this is something Boost.Python does, but only under certain conditions). But that only works when the container is a container of shared_ptr, and it has the disadvantage that you have to lock the GIL when a shared_ptr goes out of scope in order to be threadsafe (something Boost.Python notoriously does not do).

            Show
            jbosch Jim Bosch added a comment - - edited Also, I don't think I can fix the non-Pythonic nature of .as_list() or .as_dict because that would require storing a pointer to the Python objects in the DoodadSet as well, which kind of defeats the purpose of wrapping the container. But I might be wrong here. I think this is basically right. The closest you can get to the behavior you'd want in Python is to construct the C++ shared_ptr by putting a PyObject in the shared_ptr 's deleter instead of putting a shared_ptr in the PyObject (this is something Boost.Python does, but only under certain conditions). But that only works when the container is a container of shared_ptr , and it has the disadvantage that you have to lock the GIL when a shared_ptr goes out of scope in order to be threadsafe (something Boost.Python notoriously does not do).
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            I have implemented the requested fixes to the code and documentation and it is ready for a final review.
            Note that I haven't yet described the split into MutableDoodad + ImmutableDoodad in the documentation.
            I will wait with that until you decide if this is a better solution.
            If not then I will revert that commit.
            Please feel free to fix the spelling and grammar in the technote.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - I have implemented the requested fixes to the code and documentation and it is ready for a final review. Note that I haven't yet described the split into MutableDoodad + ImmutableDoodad in the documentation. I will wait with that until you decide if this is a better solution. If not then I will revert that commit. Please feel free to fix the spelling and grammar in the technote.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Two minor actions remain on this ticket:

            • grammar fixes in technote and;
            • merge of code branch on private github followed by move to LSST repository.

            Both of these will be done by Jim, so I'm closing this ticket now.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Two minor actions remain on this ticket: grammar fixes in technote and; merge of code branch on private github followed by move to LSST repository. Both of these will be done by Jim, so I'm closing this ticket now.

              People

              Assignee:
              pschella Pim Schellart [X] (Inactive)
              Reporter:
              swinbank John Swinbank
              Reviewers:
              Jim Bosch
              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.