-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation of DFT-D4 in the dftb+ infrastructure #314
Conversation
A few remarks at this early stage already:
|
All right, the D4(EEQ) can be hidden behind the usual dispersion interface, for GFN2-xTB the D4 needs a tighter integration to the SCC, but this is an issue for the GFN2-xTB implementation, not for DFT-D4. |
@aradi I hope you are fine with compiling the DFT-D4 parameter file into |
Do we have any other possibilities? 😉 I think we can live with it it, although one could think about a more dynamical solution, where the data is read from disk on demand. The only question were then, how to find the external data reliably during run (hard coded paths derived from the install prefix?) |
4a58e91
to
cb932df
Compare
I tested my implementation against the reference implementation and the results match so far. I still need to have a closer look on the strain derivatives and the Ewald summation to make sure everything works as expected. I will start with adding tests and detailed documentation soon and will be happy to address style issues. |
@awvwgk Sebastian, short question: are we supposed to look at it and consider it for merging or is this still considered to be work in progress? |
@aradi, from the implementation side I am done with this feature. I still need to figure out how and where to add tests and docs, but was occupied otherwise this week, so I couldn't really look into it yet. |
@aradi now also the docs and tests are done, at least I hope so. I am looking forward to hear your comments. |
688b4ca
to
7e31dc7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not through the entire PR yet (several parts of dispdftd4.F90 are still missing), but submit the partial review, so that we can start discussion.
Two additional general points:
- We should have a discussion, whether zeroing at allocation should be generally adapted or not. I can see advantages (stable behaviour on all systems, even if an algorithm is buggy) as well as disadvantages (debug features like initializing with NaN at allocation won't work any more, so finding bugs is more difficult).
- The manual should relate the input parameters with the symbols in the reference paper.
@aradi Thanks for the review. I carried the habit of zeroing at allocation along from previous projects, since I usually zero variables anyway. I can remove all |
I was waiting for the second half of the review, but didn't expect to get it patched in directly, so many thanks to you. I will look through the code again next week and see what I can do to move this forward. Should I add the D4 damping parameters already? |
Sorry, I thought in case of stylistic changes it is easier to push directly as to describe them. I should have, however, probably made a PR against your branch instead of pushing directly, so that it is less invasive. Anyway, for me this is almost ready to go, apart of the S9 parameter, which does not pop up in the equations in the manual, but have to be set somehow in the input. As for the D4 damping parameters, yes, adding them to the manual would make sense, for sure. |
Not a problem at all, that's why I allowed “edits from maintainers” in the first place. |
Oh, I see S9 is already in the manual. However, I think we should describe it more, as it is not clear from the first glance, that it should be either 0.0 or 1.0 depending whether 3 body terms should be included. Or do any intermediate values make sense? If not, maybe we should make it a logical ( |
Somebody might want to try 0.9 or 1.1 for the |
So there are two comments left. Than we should be ready to go.
|
As for the MPI, I updated the test diamond test to be big enough to be tested with MPI up to 4 procs. That works OK. Of course, the DFT-D4 part itself is not MPI-parallelized. At some point, we should think about it, as for a 64-diamond cell the D4-calculation takes order of magnitude longer than the rest... Leave the charge steepness as freely adjustable parameter. |
That's caused mainly by the ATM term, there are quite a lot triples. Usually it is better (meaning faster) to disable it with SQM methods. |
@bhourahine, this is an artefact from the numerical differentiation. Just move the atom with the spurious gradient contribution a bit and check again, the artefact will be gone. diff --git a/geo.gen.template b/geo.gen.template
index 79ea3e72..755002a0 100644
--- a/geo.gen.template
+++ b/geo.gen.template
@@ -2,7 +2,7 @@
Ga As
1 1 0.00 0.00 0.00
2 2 0.17 0.25 0.25
- 3 1 0.50 0.62 0.00
+ 3 1 0.50 0.42 0.00
4 2 0.75 0.75 0.32
5 1 0.38 0.00 0.50
6 2 0.75 0.10 0.75 |
@bhourahine could you try it with a stepsize of 1e-3 (instead of the 1e-5) with the critical geometry? I made the experience, that the agreement between numerical and analytical forces degrade for D4 if the stepsize goes below 1e-3 Ansgström. I am still not sure, though, where it comes from. My guess would be more on numerical instability / noise in the D4 calculation as problems with the finite difference calculation. (Without D4 I get agreement up to 1e-9 with a step size of 1e-5...). |
@aradi I tried the larger step size as well, without a significant change in results. |
@bhourahine With the modified geometry we arrive at:
We should be able to compare to the |
We should discuss this at the devel-meeting in detail. There is still the option to use the same Wigner–Seitz ansatz like in |
- keywords for DFT-D4 in hsd parser - molecular/periodic EEQ charge implementation - derivatives and strain derivatives of ATM - strain derivatives for Ewald summation - CN and charge implementation verified against dftd4 - dispersion energy and gradients verified against dftd4 - added setup for reciprocal lattice and k-point mesh - new input variables for Ewald parameters in DFT-D4 - added tests and documentation for D4 model
- Doxygen markup for subroutines - Minor tex and bibtex fixes - Minor comment changes and doxygen - Fixed header length to usual line length - Minor punctuation changes
- Fix allocation rank and dimension - Remove source statements - Fix wording in manual - Fix dot in equation
- Fix: Double counting of CN derivative in D4 dispersion - New test for charged system with DFT-D4 - Fix wrong EEQ charges for periodic case - Fix the sign error in the CN strain derivatives - Use omp default(none) instead of default(shared) - omp parallelisation for DFT-D4 - Remove usage of elemental subroutine as array operation
Added some missing variable comments, fixed a comment header and changed various left sides of matrix assignments from x = to x(:) =
I cleaned up the branch and rebased it against master. Also, I updated the legacy make build system. |
The fix for the old make system is around line 135 of prog/dftb+/make.common add in DFTD4L_INCS += -I$(ROOT)/external/dftd4refs If everyone is happy, we can then merge. |
prog/dftb+/make.common |
dftb+
MPI parallel