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

Added HF alias for HF-type exchange (EXX) to allow this GGAKEY option… #14

Closed simensr closed 7 years ago

simensr commented 7 years ago

… in lsdalton

simensr commented 7 years ago

The CI fails for this simple commit. I fail to see any correlation here. Is the CI working properly, or did I do something wrong here?

How can this be resolved?

bast commented 7 years ago

I will check it ASAP.

simensr commented 7 years ago

Great, thanks!

bast commented 7 years ago

OK your change has nothing to do with this error. This problem is tracked in #12.

bast commented 7 years ago

I am not sure whether information about HF exchange needs to be passed to and tracked in XCFun at all since it should not affect computation within XCFun. XCFun can be completely ignorant about the HFx admixture. I guess the advantage of passing this information is that it simplifies functional parsing by delegating it to XCFun. I am pointing this out because in XCint (which uses XCFun) I made the decision to not know anything about HF exchange and let the client (LSDalton) track and compute it. This may complicate a bit the parsing of the functional but it separates concerns. I am not sure what the better solution is.

simensr commented 7 years ago

I forgot to get back on this one.

Well, clearly the HFx admixture can be handled by the host program. For lsdalton this choice is historical, and the input handling (except for some cam-b3lyp testing) was handled in Pawels code, and the interface to XCfun has been made to mimic it, and is attained through xc_get(XCFUNfunctional,'EXX',hfweight). If it is not handled in this way, we need a separte list for combined functionals on the lsdalton side, rather than having them on the xcfun side (in aliases.cpp). If only lsdalton use this list, it can of course be moved to the lsdalton side.

Either way, for the lsdalton release I hope we can proceed with the merge, dispite the failing test cases (or the choice of HFx handing).

bast commented 7 years ago

The tests are not the problem. They are broken and need to be fixed independently. I see your argument about not duplicating functional definitions. Duplicating definitions is error prone and probably a larger problem than incomplete separation of concerns therefore I vote +1 for this merge.

simensr commented 7 years ago

Thanks Radovan, But, should I merge it myself, or should we have a wote here;-) Simen

bast commented 7 years ago

I think it would be good if we formalize the routine of integrating PRs. This is to avoid that we who have push access push through a change which may not be ideal. I am thinking that perhaps two "senior-enough" people should up-vote and then it can be integrated. I would also like to have @uekstrom 's opinion on this. Want to avoid the situation where we inadvertently derail the code from the "right path".

simensr commented 7 years ago

It is unclear to me to what extent Ulf is allowed to contribute to this discussion. He is bound by the contract with his new employee to stop working on former projects.

@uekstrom, in what way can you participate on this project, and who should make decisions about the future of xcfun/dftlibs?

bast commented 7 years ago

I think it is OK if we make the decisions in future without Ulf (although I would of course prefer him to be involved) but it would be good to agree on a policy with Ulf for decisions.

bast commented 7 years ago

We have now a policy in place: https://github.com/dftlibs/xcfun#contribution-guide and with this I am accepting this PR.