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

Fix python bindings #67

Closed chjacob-tubs closed 6 years ago

chjacob-tubs commented 6 years ago

This is my attempt at getting the SWIG Python bindings back to work. It does the job for me, but I am sure the setup is not perfect (maybe a cmake expert can have a look, see "possible improvements" below). (Mostly fixes #63)

Changes:

Possible improvements:

codecov-io commented 6 years ago

Codecov Report

Merging #67 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #67   +/-   ##
=======================================
  Coverage   42.29%   42.29%           
=======================================
  Files          78       78           
  Lines        1785     1785           
=======================================
  Hits          755      755           
  Misses       1030     1030

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 16c90a1...18bc70e. Read the comment docs.

robertodr commented 6 years ago

I am not familiar enough with SWIG to give comments off the top of my head. I'll check out this branch locally, test it out and give a more informed review.

bast commented 6 years ago

I think short-term this is fine. Long-term either pybind11 or CFFI since they provide a thinner interface.

chjacob-tubs commented 6 years ago

Thanks for the comments. Let me have another look before merging this, I will try to fix at least the copying/installation part ...

robertodr commented 6 years ago

Apologies that it took quite a bit to review this one. Being in another timezone did not compose well with being unfamiliar with SWIG. Still, great learning opportunity for me!

chjacob-tubs commented 6 years ago

I put some more work into this and now used UseSWIG instead of the manual setup that was used before. Please have a look, I think this is much nicer now. However, I didnt the RPATH stuff to work so I am still linking to the static library. To me, this is not a problem, but maybe you want to fix that.

I tested on MacOS and Linux with gcc. I am not sure whether switching to a different compiler will cause problems when building the Python shared library, because the Python libraries are usually built with gcc.

I also fixed the copying part, so the xcfun Python module can be used directly from the build directory.

chjacob-tubs commented 6 years ago

Sorry for removing FinfPythonLibsNew - I thought that was something obscure that I added at some point. I stand corrected. The target_include_directories is indeed required, but I made the linking part a bit nicer

robertodr commented 6 years ago

@bast I think once this is in we can:

  1. Branch off release/v.2.0
  2. Officially tag v2.0.0
chjacob-tubs commented 6 years ago

@robertodr @bast Let me fix the Python tests (#43) before doing the v2.0 release.

robertodr commented 6 years ago

Oh yes, of course! Thanks for reining in my enthusiasm :) Merging this one and closing #67