search

dftlibs / xcfun

XCFun: A library of exchange-correlation functionals with arbitrary-order derivatives
https://dftlibs.org/xcfun/
Mozilla Public License 2.0
57 stars 32 forks source link

Bind to Python with pybind11 #96

Closed robertodr closed 4 years ago

robertodr commented 4 years ago

Description

I've redone the Python bindings using pybind11. The bindings are now more transparent and easier to read/extend than the SWIG bindings were.

If pybind11 is not around, it will be fetched. This trick requires use of CMake 3.11 at least.

Types of changes

Questions

Status

codecov-io commented 4 years ago

Codecov Report

Merging #96 into master will not change coverage. The diff coverage is 75%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #96   +/-   ##
=======================================
  Coverage   49.46%   49.46%           
=======================================
  Files          77       77           
  Lines        1785     1785           
=======================================
  Hits          883      883           
  Misses        902      902
Impacted Files Coverage Δ
src/xcfun.cpp 71.99% <75%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ca137ba...db470d4. Read the comment docs.

stigrj commented 4 years ago 
./setup --pybindings

gives the following error

CMake Error at python/CMakeLists.txt:8 (find_package):
  Could not find a package configuration file provided by "pybind11"
  (requested version 2.3.0) with any of the following names:

    pybind11Config.cmake
    pybind11-config.cmake

  Add the installation prefix of "pybind11" to CMAKE_PREFIX_PATH or set
  "pybind11_DIR" to a directory containing one of the above files.  If
  "pybind11" provides a separate development package or SDK, be sure it has
  been installed.
robertodr commented 4 years ago

Yeah, you need to have pybind11 installed. I still have to set up the superbuild.

On Tue, Oct 29, 2019, 08:58 Stig Rune Jensen notifications@github.com wrote:

./setup --pybindings

gives the following error

CMake Error at python/CMakeLists.txt:8 (find_package): Could not find a package configuration file provided by "pybind11" (requested version 2.3.0) with any of the following names:

pybind11Config.cmake
pybind11-config.cmake

Add the installation prefix of "pybind11" to CMAKE_PREFIX_PATH or set "pybind11_DIR" to a directory containing one of the above files. If "pybind11" provides a separate development package or SDK, be sure it has been installed.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dftlibs/xcfun/pull/96?email_source=notifications&email_token=AA4JOEMBJAQ6NAHJITXH74TQQ7UIXA5CNFSM4JFNDZWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECPSC2Q#issuecomment-547299690, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4JOEKSKYB34PJD33EOJLDQQ7UIXANCNFSM4JFNDZWA .

stigrj commented 4 years ago 
Each download failed!

    error: downloading 'https://github.com/pybind/pybind11/archive/v2.3.0.tar.gz' failed
         status_code: 1
         status_string: "Unsupported protocol"
         log:
         --- LOG BEGIN ---
         Protocol "https" not supported or disabled in libcurl

  Closing connection -1
robertodr commented 4 years ago

Ehm... It works on Travis :man_shrugging: Where are you trying to build?

stigrj commented 4 years ago

It's the same

./setup --pybindings

...sorry, I read "what"...

I'm building on my Ubuntu

robertodr commented 4 years ago

I cannot reproduce it. Can you try to cURL pybind11 by hand?

stigrj commented 4 years ago

I'm able to run the command in .ci/pybind11.sh

curl -Ls https://github.com/pybind/pybind11/archive/v${pybind11_VERSION}.tar.gz | tar -xz -C pybind11 --strip-components=1

Is this the same command FetchContent_Populate runs?

robertodr commented 4 years ago

I don't think it's the exact same, but something similar.

robertodr commented 4 years ago

Googling turned this one up: https://github.com/ruslo/hunter/issues/972#issuecomment-323687499

stigrj commented 4 years ago

Argh, the annoyance

robertodr commented 4 years ago

Gentle nudge here. It'd be good to get this merged so I can move on with conda packaging and redoing the Fortran interface with iso_c_binding

bast commented 4 years ago

I will review today ...

robertodr commented 4 years ago

Thanks :octopus:

chjacob-tubs commented 4 years ago

Sorry, was busy / travelling the last two weeks. I will try to test over the weekend.

robertodr commented 4 years ago

Yes, this should be a beta release. We can do rc when the Fortran bindings are refurbished.

chjacob-tubs commented 4 years ago

It works on my Mac, but I somehow dont get it to work on Ubuntu. Setup now fails with CMake Error: Could not open file for write in copy operation /home/christoph/sources/xcfun/build/include/XCFun/XCFunExport.h.tmp CMake Error: : System Error: Not a directory CMake Error at /home/christoph/sources/cmake-3.15-install/share/cmake-3.15/Modules/GenerateExportHeader.cmake:394 (configure_file): configure_file Problem configuring file Call Stack (most recent call first): /home/christoph/sources/cmake-3.15-install/share/cmake-3.15/Modules/GenerateExportHeader.cmake:410 (_do_generate_export_header) src/CMakeLists.txt:4 (generate_export_header)

Any idea what the problem might be?

robertodr commented 4 years ago

Yes, I made a silly CMake mistake. Should be fixed in the latest commit.

chjacob-tubs commented 4 years ago

OK, now it works! (One minor issue: It seems that CMake does not discover the system-installed pybind11 but always downloads and builds itself. Thats fine for me, I actually dont know how / where CMake is looking for pybind11)

robertodr commented 4 years ago

How was pybind11 installed? I can look at this later next week.

chjacob-tubs commented 4 years ago

On Ubuntu both with apt-get and with pip, an the Mac with macports

robertodr commented 4 years ago

The PyPI package does not install the CMake support files. The package from apt-get might be too old? What version is it?

chjacob-tubs commented 4 years ago

OK, I got it to work, I guess that was the problem.

chjacob-tubs commented 4 years ago

Should we merge this now? How are you handling this / who is pushing the button?

robertodr commented 4 years ago

Sorry, should have been clearer. I wanted to give @aspgomes one more week for comments, then anyone with write access can push the button (I usually prefer not to self-merge)