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

Make xc_get() part of the API #79

Closed stigrj closed 5 years ago

stigrj commented 5 years ago

Questions

Is there any reason why this function is not part of the C++ API?

int xc_get(xc_functional fun, const char * name, double * value);

Motivation and Context

I would like to be able to retrieve the amount of exact exchange from XCFun. To me the only reason to keep the exx parameter in XCFun is to keep track of it for the host program, but currently I don't see any way to fetch this information (correct me if I'm wrong). I would like to avoid duplication of this parameter.

Alternative solution

Add specific function to fetch exx:

XCFun_API double xc_get_exx(xc_functional fun)

Description

Added XCFun_API to the xc_get() function in xcfun.h.

Types of changes

Status

stigrj commented 5 years ago

Travis fails on an unrelated (?) Python issue.

robertodr commented 5 years ago

No particular reason to not have it exposed other than it was not used in the examples and tests. It would be good if:

  1. You could add a test for it.
  2. Document the API function with a Doxygen string. In particular, knowing which names are valid as second argument.

Travis failures are due to Pipenv vagaries. I think this is a minor change and can be pulled in and I can later look at greening Travis up. @bast what do you think?

bast commented 5 years ago

If this PR can wait a day or so then I would prefer if we fix the pipenv issue in a separate PR and rebase this on top of that. Does that sound OK?

stigrj commented 5 years ago

NP, I will work on @robertodr 's points in the mean time

bast commented 5 years ago

Good - I am looking that the pipenv side of things.

bast commented 5 years ago

Pipenv issue followed-up in #80.

robertodr commented 5 years ago

Yep, much better. Thanks all!

bast commented 5 years ago

I am working on the pipenv issue and will ping you here once this patch is ready to be rebased.

bast commented 5 years ago

81 is ready to go. Once merged, please rebase this patch.

stigrj commented 5 years ago

Rebased and ready to go :crossed_fingers:

robertodr commented 5 years ago

There are some genuine test failure now:

Numerical check failed: B3LYP contains  0% FOO
 got      1.1999998092651367e+00
 expected 0.0000000000000000e+00
 Absolute error is 1.2000e+00, relative error is inf
stigrj commented 5 years ago

Sorry, I thought even undefined functionals returned a 0.0 value.

codecov-io commented 5 years ago

Codecov Report

Merging #79 into master will increase coverage by 0.59%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   44.37%   44.97%   +0.59%     
==========================================
  Files          78       78              
  Lines        1839     1839              
==========================================
+ Hits          816      827      +11     
+ Misses       1023     1012      -11
Impacted Files Coverage Δ
src/xcfun.cpp 50.23% <ø> (+2.54%) :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 025d2e8...8372679. Read the comment docs.

robertodr commented 5 years ago

No problem. And coverage went up by a whopping 0.59%! Thanks.