Code review comment for lp:~alesster/siesta/trunk_new_xc

Revision history for this message
Alberto Garcia (albertog) wrote :

Sorry about the delay in handling your merge request for new vdW options in SiestaXC.
The delay was motivated by two issues:

1. The Gitlab transition.
2. The development of a stand-alone library, ligGridXC, based on the code in SiestaXC.

LibGridXC is being developed at

    https://gitlab.com/siesta-project/libraries/libgridxc

and is "the future", in the sense that it will be required as a external library for Siesta very soon, once we merge in the next few weeks the PSML branch.

LibGridXC has the same functionality of SiestaXC, with some streamlining, and, in addition, offers an interface to LibXC. This interface is optional now, but very likely it will be made compulsory in the future.

As I understand it, your merge request:

* Adds a few more vdW flavors.
* Implements a new semilocal SG4 functional that is needed by some of the new vdW flavors.

I have no problem with the content of your merge request. I think it should definitely go into libGridXC, but I am not sure whether to integrate it also into the current code base (based on SiestaXC) which is "going away" soon.

In any case, I would request that you make new merge requests within Gitlab:

1. One for libgridXC.
2. (If you think it is worth it) Another one for the "master" branch of Siesta:

             https://gitlab.com/siesta-project/siesta

Note that, as part of the Gitlab transition, the Launchpad branch that you proposed for merging is now available in git form in:

             https://gitlab.com/siesta-project/siesta-legacy/alesster_trunk_new_xc

As a further comment, note that the SG4 functional is already provided by libXC. However, the current way in which the semilocal pieces of the vdW functionals is handled in ligGridXC needs to be checked for compatibility with libXC. It is not as easy as passing the appropriate LIBXC-XXXX string to ggaxc, since the functional object needs to be initialized in advance. So I would keep the explicit implementation of SG4 for now.

« Back to merge proposal