grimme-lab / xtb

Semiempirical Extended Tight-Binding Program Package
https://xtb-docs.readthedocs.io/
GNU Lesser General Public License v3.0
580 stars 144 forks source link

API object to manipulate solvation model #336

Open awvwgk opened 4 years ago

awvwgk commented 4 years ago

Is your feature request related to a problem? Please describe. Currently the C-API to add and remove a solvation model is rather limited. With the new ALPB model and parameters it is missing the flexibility necessary to match the standalone program.

Describe the solution you'd like Create a new void pointer type for the solvation model in the C-API to allow an incremental construction of the solvation input data and expose an C-API call to attach this model to the calculator.

Additional context Resolving this issue increments the minor version of the C-API.

TyBalduf commented 2 years ago

Am I understanding this correctly that (at the moment) the C-API always uses GBSA rather than ALPB? To use ALPB currently, could I just toggle input%alpb = in calculator.f90::setSolvent_api to .true. before compiling?

pultar commented 1 year ago

Is this still work in progress? I would need this feature and haven't seen the new solution in master.

Am I understanding this correctly that (at the moment) the C-API always uses GBSA rather than ALPB? To use ALPB currently, could I just toggle input%alpb = in calculator.f90::setSolvent_api to .true. before compiling?

Pretty sure it's GBSA, at least it gave me (nearly) identical energies and failed for unparametrized solvents:

GBSA solvent:
    Reference: -8.383938647042
       Energy: -8.383938647161

I tried the naive change in the API as suggested by @TyBalduf

https://github.com/grimme-lab/xtb/blob/444bb99ac2f6fc722ec6fbe897033862f7be5ded/src/api/calculator.f90#L393

and got a deviation of energies compared to a calculation with the binary

ALPB solvent:
    Reference: -8.384076843892
       Energy: -8.383934553534
Energies NOT identical

@awvwgk @MtoLStoN Do you have any suggestions how this issue could be resolved? Let me know if I could help.

TyBalduf commented 1 year ago

@pultar I realized not too long after that there is one more change that needs to be made. ALPB is meant to be run with the p16 interaction kernel, so you would also have to change the next line from input%kernel = gbKernel%still to input%kernel = gbKernel%p16. In my testing, this was enough to get consistent results from the API and executable.

MtoLStoN commented 1 year ago

@TyBalduf is right. We use a different interaction kernel for ALPB than for GBSA. The C-API is not actively being developed right now, but we should update it to reflect new features sooner or later.

pultar commented 1 year ago

Thanks @TyBalduf and @MtoLStoN

The change works!

I guess it would break the API for other users if we added an option to switch between solvents? Unless the function was overloaded like this:

https://stackoverflow.com/questions/479207/how-to-achieve-function-overloading-in-c

// Edit: the change could look something like that: https://github.com/pultar/xtb/commit/9b260af073157975545d03b83f634e92f8b5c008