randomize_grid does not randomize the PP grid as expected #4362
Comments
|
Thanks Mark. Indeed this seems like a bug, since we shouldn't rely on grid density or symmetry to get the correct result. It should be correct even if we use one point, which it will not be based on your plot. The functions in question are at https://github.com/QMCPACK/qmcpack/blob/develop/src/QMCHamiltonians/NonLocalECPComponent.cpp#L868 (we have two variants) It is also interesting that we use 3 random numbers for this. |
|
This is how the code looked when randomize_grid was introduced in Mar. 2005 (commit: a9b5aad, file NonLocalPPotential.cpp): Notice 1) more comments, 2) the additional factor of 2 in |
|
The factor of two was removed when |
|
The overall method may be closely related to details contained in Murnahan (https://doi.org/10.1073%2Fpnas.36.11.670) and Miles (https://doi.org/10.2307/2333716). |
|
Is the density uniform if you simply propagate a single vector many times with the uncorrected code? |
|
The plot shown (and the script) applies many random rotations to a single vector (unit vector along the z-axis). Do you mean some other scheme - like applying random rotations to random vectors? I didn't measure the resulting distribution in any more detail than just visual inspection. |
|
It is quite likely that this is a very small bias in practical calculations since we have large default integration grids and the points used in the integration grids (which are referenced to electron positions) will vary considerably unlike the fixed (0,0,1) in this test. However we should fix it since there are cases when there could be a bias, including for coarse grids. That large runs are OK can be tested since we will see if the statistical tests change meaningfully. Tests have also been done vs Hartree-Fock orbitals in several cases, and the problem did not show up. The fix unfortunately likely requires updating a great many deterministic tests. Mark, can you please check the situation with your fix? Then we will know how much work is entailed. We also clearly need a unit test that calls this function 10^6 times and verifies that we can evaluate a simple numerical integral correctly. e.g. We would presumably fail to integrate the surface area of a sphere with the current code and single point grids. |
|
Using a real, full precision build with the corrected grid randomization:
(I had left some stats for the tests earlier, but deleted them after I discovered I had disabled the grid randomization entirely) |
|
I agree with Paul. That we haven't seen a discrepancy before now in reference testing suggests any bias is very small. Likely worthwhile to explicitly test with a long DMC run (longer than a "long" test) of a small molecule (e.g. H2O) before and after the fix. |
Extract the storage and generation of the random rotation matrix to make it easier to test. Fix the issue in QMCPACK#4362 where the distribution of the rotations does not cover the entire sphere.
|
To gather statics, I am currently running 10 sets of all the long tests, with and without the proposed fix. This will take ~a day to run. Added 14 Dec: Had to restart due to not setting adequate test timeouts. Rerunning with a smaller ensemble. So far it looks like the main breakage is in the deterministic tests. |
|
@markdewing put the matrix rotater in Numerics and remove all physics references? This is only maths/numerics. |
|
I had an idea for incremental fixing of the deterministic tests. One of the features I want for testing is to be able to turn off the grid randomization from the input file. Implementing that feature would not cause a breaking change. Input files and deterministic tests could be updated individually to have the grid randomization turned off. Then applying the fix to the randomization wouldn't break the tests. The downside is then the tests would need to be updated again if we want grid randomization to be included in the test results. But maybe only a subset of them would need to get this second update. |
|
Under what circumstances is the grid reoriented today? Some QMC codes reorient every single evaluation. |
It looks like it is every evaluation...for example see NonLocalECPotential::evaluateImp. gets randomized a few lines into the funciton |
|
Test results from 4 runs of a real CPU build gcc no mpi, ctest -R 'deterministic|long'. No short or converter (etc.) tests run. So, an average of 5 failed tests per run, and only long run failures. Now with the proposed fix, too many deterministic tests fail than should be listed here: An average 275 failures per run. An average 4 failed long runs. This is actually one better than the unfixed code, and it seems that there were problems unrelated to the fix (SIGHUP?) with both long-diamondC_1x1x1_pp-vmc_sdj_excited-1-16 and long-diamondC_1x1x1_pp-vmc_sdj_meshf-1-16, explaining the reported samples failures. And none of the long nonlocal checks fails, so we know that the nonlocal pseudopotential energies are consistent. Therefore for the elements and levels of statistical accuracies of our test set it is fair to conclude that the problem averages out but our deterministic tests are highly sensitive to it. |
|
@prckent close? |
I would expect the function
NonLocalECPComponent::randomize_gridto randomize the grid origin across the entire sphere.But it does not - it's missing some coverage around the "poles". See the attached graph.
The root cause is the initialization of
cthfrom a random number. It is currentlycth = myRNG() - 0.5. I think it should becth = 1.0 - 2*myRNG().I'm not sure this issue has a practical impact, since the rotation is only meant to change the origin of the spherical integration grid and not cover the sphere by itself. If the grid is sufficiently dense, the other points will cover the missing regions from the origin. (But I didn't actually check this)
There is another minor annoyance in that when initializing the current code with all zeros for the rng values, the result is not the identity rotation. With the corrected initialization, using zeros does result in the identity rotation.
I tried to find the origin of the formula in
randomize_grid. Nothing online seemed to match, and I couldn't recreate it through multiplying 2x2 rotation matrices (though that search was not exhaustive).Plot of random rotations applied an input vector (0,0, 1)
The script used to generate the plot
check_grid_randomize.tar.gz
The text was updated successfully, but these errors were encountered: