-
Notifications
You must be signed in to change notification settings - Fork 141
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
Incorrect ion-ion energy in some periodic systems #2105
Comments
Initial report with files https://groups.google.com/forum/#!topic/qmcpack/3Hkt5sWY_e4 |
Fast to run tests were added for this bug in #2116 - bug-deterministic-grapheneC_1x1_pp-vmc_sdj_z10-1-1-ionion and bug-deterministic-grapheneC_1x1_pp-vmc_sdj_z30-1-1-ionion . Both fail. The reference value is currently the QE value. |
I'm just going to use this as the dumping ground for ongoing tests: Ewald in QE converges really quickly. I'm doing config #3 from the forum--QE is initialized for a 100 atom cell corresponding to the 5x5x1 tiling... this is to keep the comparison between the supercell QMCPACK calculation and QE's ewald summation apples-to-apples. Ecut (Ry) | Ewald Energy (Ry) | Ewald Energy (eV/cell) This is in contrast to QMCPACK's Ewald3D and Optimized Breakup handlers: r_c*k_c | Ecut (Ry) | Ewald 3D (eV/cell) | OptBreakup (eV/cell) To get Ecut(Ry), I used k_c^2/2 (energy cutoff in Hartree) and converted to Rydberg (x2). |
Test 2: Comparison of Short Range, Long Range, and Background Ewald Terms between QE and QMCPACK So I think the bug is in the computation of the long-range piece of the potential only. Here's the evidence to back this up. I hacked QE to spit out the following.
Here are the columns:
For the QMCPACK comparison, I looked at just the first row in the above table. I set lr_dim_cutoff to what's listed in the first row, and I hard-coded the Sigma in the EwaldHandler3D to be that given in the table. Note that QMCPACK chooses this differently from QE, hence why this was necessary. Running this in the same geometry as the QE run, I get the following for the energy: To make this explicit, I'm showing the difference between QMCPACK and QE for the various terms:
**Basically, I suspect the bug is just in the long-range computation of the coulomb potential, and probably not in the actual breakup procedure. Could be the G-vector initialization/manipulation, which wouldn't surprise me given PR #2087. Could be improper initialization/filling of the v_k (long range piece of coulomb potential). Maybe S(k) is not computed correctly, but I doubt this. ** |
I think I found the bug. In these large cells, there's a misidentification k-vectors with k-shells. To save time, the ewald summation is done over k-shells instead of looping over the individual K-points. So the long-range piece in Fourier space v_lr(k) is stored in a kshell-based array corresponding to v_lr(|k|) (If you go digging in the code, this is Fk_symm[]). What I'm doing is taking the list of k-vectors given in KList.kpts_cart, and I'm comparing the stored value of v_lr(k) (this is Fk[] in the code, which in turn is set from Fk_symm[] since hey, all the Fk values in the same shell should be identical) to the one I would compute directly using v_lr(k) given by Ewald for that k-vector. Here's what I get.
Here's what the columns are
Oh snap. #22 and #23 are noticeably different. I cut the list off instead of showing the results for all 75000 k-vectors, but there are blips in the list every 80-100 kvectors on average with tapering off magnitudes. So let's take a look at those k-vectors around #22 and #23.
Looking at i=22 and 23, we see that these k-vectors are misidentified as belonging to kshell #6 despite the fact that their lengths are very slightly off their neighbors. |
Confirmed the bug. The tolerance is effectively set in src/LongRange/KContainer.cpp line #256 so that kvectors differing by less than 0.001 in |k|^2 are treated as being in the same k-shell (Also a comment on line #253 to this effect). Increasing the effective tolerance to 1e-7 by jacking up that integer multiplication gives us the following values for the ion-ion interaction computed by the EwaldHandler3D routine:
Now the differences from QE's Ewald:
Boom. |
Now that #2125 is merged, the development version will hard abort if the computed ion-ion energy differs significantly from an independent reference Ewald computation performed to a specific energy tolerance. The graphene z10 and z30 tests currently both correctly fail at this check. |
I would feel a lot more comfortable with this if we were casting to something with a larger range than
I don't know what range of values we are guaranteed that k^2 will fall in for all cells, but using an integer type with larger range should allow the safe use of a larger precision multiplier (1000 here) without loss of precision or risk of overflow. |
Could you provide delta with respect to the reference implementation added by Jaron? I still feel LR is larger than I would expect. |
To summarize the situation: a bug was found in the Optimized Breakup implementation and fixed (#2137), but both this implementation and reference Ewald are also found to have slow convergence for anisotropic systems. A reference Ewald check on the ion-ion term has been added to the production code to catch this condition. Tests have also been added that currently fail due to very slow convergence of the underlying algorithm. A better algorithm and implementation is needed to (1) deliver good accuracy and (2) do so fast enough. #2185 is one possibility. |
Improvements will be investigated in #2185 . Closing this since bugs have been fixed and safeguards introduced. |
Users are still getting bitten by this in practice quite often (perhaps each and every one who tries a quasi-2D system). I recommend we detect quasi-2D input (easy) and switch the default to |
Hi guys, I am also hitting this bug for a periodic system of 32 water molecules. Here is my input for a jastrow opt:
and the error is:
Please @jtkrogel let me know what I can do to correct this!! |
Interesting. This is the first time the issue has been seen in a dense system in PBC's. You should be able to fix this by switching to:
This will use an alternative method to do the Ewald sum (safe but more expensive). You can access this via Nexus by setting |
@kryczko see above. |
right on! I'll give that a go and post back. Thanks so much @jtkrogel !! |
I'm reopening this issue. To other developers: this provides yet another datapoint supporting a switch of the default algorithm to the direct Ewald implementation. In the event of sub-optimal performance, the focus should be on speeding up this implementation. Separately I will note that no effort was made to improve the robustness of the Natoli-Ceperley implementation. It could be as simple as enforcing a tolerance on the convergence of the SVD singlular values used to arrive at a compact representation of the real space basis. |
|
I tried running with the parameters given by @jtkrogel above, but still see same error. Please let me know if there is something else I am missing. |
gotcha -- just have to wrap the coordinates. |
This is an important bug that exists in QMCPACK dating to at least mid-2015, likely earlier.
First noticed for strained phosphene and also reproduced in A-A stacked graphene, for some configurations the ion-ion coulomb energy is incorrectly computed in these periodic systems. The errors can be in eV. To date dense solids appear to be correct - and these are already checked nightly. We are investigating the exact cases when the problem occurs and will update this issue. The bug could be in "plumbing", implementation, or simply unexpected behavior of the implemented method (Natoli/Ceperley).
The ion-ion term should of course be exactly the same as reported by the code that generated the trial wavefunction e.g. Quantum Espresso. For the time being we recommend making cross-checks.
The text was updated successfully, but these errors were encountered: