Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python Bindings for Cork #17

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Python Bindings for Cork #17

wants to merge 5 commits into from

Conversation

stevedh
Copy link

@stevedh stevedh commented Nov 17, 2014

This is mostly to see if you're interested in supporting python bindings. I wrote some typemaps that do a pretty decent (zero-copy!) job of it if you use numpy matrices. There are test cases that verify we get the same result as calling the binary on the samples (they won't work though unless you patch initRand and just do an srand(0) there).

Before we would merge, I think there are a couple things we need to check ...

  • I'm not sure how cork allocates memory, but python is probably going to call free() on the memory returned at some point. The references to the return arrays get stolen by numpy. This should be fine/desirable, provided we make sure that numpy is using the matching deallocator from cork and that we're not leaking memory. Otherwise we'll need a copy on the way back to python.
  • I was able to get this to work on OSX 10.10, and Ubuntu 14.05 with clang++. However, the library isn't built with -fPIC, and so there could be problems if you use a different compiler.
  • The setup.py isn't self contained now -- it doesn't call make to build the library. With some more fiddling, we could make this easier to distribute with pip.

produce the same answers as calling the binary.

The main work we need to do is that input parameters may not be
properly deallocated.
    This may or may not be desirable for the non-python use case.  There's no clear way to do this with a flag, but we should probably build out the build system
This was a bit of an adventure.
@stevedh
Copy link
Author

stevedh commented Nov 22, 2014

8a2d3e6 resolves the deallocation issue; before we were indeed just never freeing the memory allocated as return values.

Also, PIC is required for building a python extension. a01b9b4 shows where to add to the Makefile; I think it's okay to leave them in there, but the makefile sort of needs more surgery than I'm willing to do right now. Making it respect CXXFLAGS would be nice though!

@chrisidefix
Copy link

Thanks for implementing this, even though I assume cork is no longer maintained, I am trying out your python bindings. Any reason why you named the package _cork (with underscore) or was this a temporary thing? I absolutely think this should go up on pypi and the actual cork lib should go into homebrew.

@stevedh
Copy link
Author

stevedh commented Sep 21, 2015

It's kind of a swig convention -- it compiles the actual c code into a shared library named _cork.so, and generates a python module named cork.py that does some type checking then calls the real call inside of _cork. So in application code, you would still "import cork" as people probably expect.

Yeah, I don't expect it's maintained; I was really just helping my buddy out with these bindings but they should be decent, if people are using the project. We found that we were wasting a lot of time dumping meshes and reloading then rather than using them in-process.

@gilbo
Copy link
Owner

gilbo commented Sep 21, 2015

Hi, yes. I don't have much time to maintain this code these days. However, it seems like various people have built extensions. I think it'd be a good idea to try to merge some of these efforts if possible. Please feel free to email me if you want to talk about someone else taking ownership of the code.

Of course, I'll be around to consult regardless--especially on the numeric/robustness aspects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants