theochem / horton

HORTON: Helpful Open-source Research TOol for N-fermion systems
http://theochem.github.io/horton/
GNU General Public License v3.0
94 stars 40 forks source link

Move from Cython to Pybind11 #236

Closed matt-chan closed 7 years ago

matt-chan commented 7 years ago

Just creating this issue to discuss. Definitely not committed to this yet.

If we move from Cython to Pybind11, we can simplify a lot of the build process and have really basic setup.py and move most of the dependency checking to CMake (which has tons of stuff built in already). I'm still reading the documentation for pybind11, but it seems to be fairly simple. It even generates documentation automatically. It won't let us write "fast Python-ish" code though.

I don't think it'll eliminate the need for python unit tests for C++ code, but it'll let us use Python unit tests for checking the binding worked and C++ unit tests for C++ code, and C++ build tools for building everything.

matt-chan commented 7 years ago

So it looks like the downside of Pybind11 is very verbose C++ code. Pybind11 is supposed to use less binding code, but that's only true when you don't use Numpy. There's no function to create a zeroed numpy array for example. You first need to create an unallocated array of specific dimensions and then write for loops to iterate over everything and zero it out. That's a lot of code for something that Cython does in 1 line. There's also a few possible errors that can be introduced at that point.

Currently we're leaning towards a hybrid CMake/Cython + setuptools build system.

  1. We would use CMake to build the C++ code (particularly all our C++ code when it links in external dependencies like libint or ATLAS) so we can get dependency resolution. The conclusion of that step gives us a shared object (residing in a known location) which can be used directly in other C++ programs, or be linked to Python.
  2. Another option of the CMake script would be to call the Cython compiler as well and provide library resolution. This results in a shared object that contains not only our C++ code, but also the Python binding code.
  3. We would use a minimal setuptools to install the files into wherever the user wants. We avoid all the setup.cfg nastiness because we already know the location of the shared objects.

There's a working example here: https://github.com/thewtex/cython-cmake-example/tree/master

tovrstra commented 7 years ago

@matt-chan Can you push your branch with the celllists pybind11 wrapper to the celllists repo? It's still useful to keep such attempts for future reference.

matt-chan commented 7 years ago

Sure, I'll do that in a bit

On Tue, 27 Jun 2017 at 09:20 Toon Verstraelen notifications@github.com wrote:

@matt-chan https://github.com/matt-chan Can you push your branch with the celllists pybind11 wrapper to the celllists repo? It's still useful to keep such attempts for future reference.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/theochem/horton/issues/236#issuecomment-311274698, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_-NTAFIDW5_INpqkElnEP3V6-rYGiIks5sIK0-gaJpZM4N0cdy .

-- Matt

Sent from my phone

matt-chan commented 7 years ago

Done!

On Tue, 27 Jun 2017 at 09:44 Matthew Chan chanmhy@mcmaster.ca wrote:

Sure, I'll do that in a bit

On Tue, 27 Jun 2017 at 09:20 Toon Verstraelen notifications@github.com wrote:

@matt-chan https://github.com/matt-chan Can you push your branch with the celllists pybind11 wrapper to the celllists repo? It's still useful to keep such attempts for future reference.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/theochem/horton/issues/236#issuecomment-311274698, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_-NTAFIDW5_INpqkElnEP3V6-rYGiIks5sIK0-gaJpZM4N0cdy .

-- Matt

Sent from my phone

-- Matt

Sent from my phone

matt-chan commented 7 years ago

Closing because this isn't a good solution.