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

Possible bug in functional definition #50

Closed ilfreddy closed 4 years ago

ilfreddy commented 6 years ago

I am trying different combinations to allocate a functional in MRChem. It seems to me that the following code snippet should work fine but the functional object is undefined. When I ask for the input length, it returns a random (and unreasonable) xc_input_length.

xc_eval_setup(this->functional, XC_N_GNN, XC_POTENTIAL, 1); string n1 = "PBEC"; string n2 = "PBEX"; xc_set(this->functional, n1.c_str(), 1.0); xc_set(this->functional, n2.c_str(), 1.0); std::cout << " nInput " << this->getInputLength() << std::endl;

The last fcn is just a wrapper for xc_input_length

Two options: either I am providing a faulty set of parameters (it does not seem to me) and then xcfun should warn me about it and exit or the choice is sane ad then there is a bug in xcfun.

robertodr commented 5 years ago

Is this behavior still occurring?

robertodr commented 4 years ago

@ilfreddy is this still an issue? If not, please close the ticket..

ilfreddy commented 4 years ago

I believe so. If I remember correctly from a discussion with @bast , the XC_POTENTIAL mode is in practice not used. Either one uses the XC_CONTRACTED mode (Dalton) or the XC_DERIVATIVES mode (MRChem). What we actually need is a XC_PARTIAL_CONTRACTIONS both for MRChem and Dalton.

chjacob-tubs commented 4 years ago

We are using the XC_POTENTIAL mode in PyADF/PyEmbed.

ilfreddy commented 4 years ago

I see. This is an old issue for me (I then realized I needed the XC_DERIVATIVES mode and disregarded the problem). Could you have a look at the snippet above and check if you see any obvious mistake from my part? Alternatively, could you show us how you allocate and initialize the functional, @chjacob-tubs?

chjacob-tubs commented 4 years ago

I will have a look and try to find time for this next week. We use the Python bindings, but of course it should also work directly in c++.

chjacob-tubs commented 4 years ago

I think you are doing things in the wrong order in your code snippet:

I am not sure about the possible combinations of input variables with XC_POTENTIAL. For GGA functionals we are using XC_POTENTIAL in combination with XC_N_2ND_TAYLOR (for the closed shell case) or XC_A_B_2ND_TAYLOR (open-shell case). You can have a look at python/xcfun.py for inspiration. In any case, for a GGA functional XC_N_GNN should not work (throw an error?) because you need the second derivative of the density to calculate the potential.

Hope that helps to solve your problem ...

ilfreddy commented 4 years ago

From what I vaguely remember (this is a 2yo issue) I had already called xc_new_functional, and I believe (again, not sure) that it does not matter the order between xc_set and xc_eval_setup, because they both simply set some parameters. But your last comment about XC_N_GNN not working seems the right answer. I assume this is then not strictly a bug, but rather an issue of detecting the conflicting choice of parameters and exiting with an error. Thanks for feedback, @chjacob-tubs! I suggest to keep this open for now, for somebody to look at.

chjacob-tubs commented 4 years ago

I checked again and the xc_eval_setup does throw an error if inconsistent options are use. The problem with the code snippet is that the GGA functional is selected after calling xc_eval_setup. This cannot be caught and is not a bug. So I am closing this issue - if problems show up you can always open a new one.