Add IonIon force observable to CoulombPotential in OBC #1820
Conversation
|
Can one of the maintainers verify this patch? |
There was a problem hiding this comment.
This looks fine to me! The main thing I would like to add is a simple unit test. Can you add one to src/QMCHamiltonians/tests/test_force.cpp? You can copy/paste either the "Ceperley Force" or "Chiesa Force" test, but swapping out the ForceCeperley object with CoulombPotential. You should have reference force values on hand.
|
Is there a reason to keep the forces="yes" requirement? I would prefer for ion-ion to be included automatically anytime a force estimator is used. |
|
@jtkrogel In the long run, no. In the short run, this allows the force components to be printed separately to scalar.dat for checking purposes, and also handles some potential one-off initializations that might be needed for forces. This will definitely need to be streamlined. |
|
I think the total force and each sub-component should be written automatically every time. I guess the conditional initialization issue requires more coordination between the force estimator and the hamiltonian components? |
That's the tricky bit. |
|
I added a unit test. Not sure if it's very classy, but it seems to do the job. I take it we are still sticking with the 'forces=yes' practice for now. Enabling/disabling the feature should be easy enough using a flag in the initialization, but the logic behind this flag should probably be re-designed in the future. I would vote for making ion-ion force the default, provided that there is a force estimator present, otherwise not. |
|
Can one of the admins verify this patch? |
|
Ok to test. |
|
Complex builds are failing. Please have a look. |
| inline void evaluateAAForces(const DistanceTableData& d, const ParticleScalar_t* restrict Z) | ||
| { | ||
| forces = 0.0; | ||
| GradType forcetmp = 0.0; |
There was a problem hiding this comment.
I can't tell for sure without reading the error log, but this will probably bite you in real vs. complex builds, because for real builds GradType will basically be a real 3d vector, whereas in complex it will be a complex 3d vector. Force should always be a real 3d vector. You should change this to PosType and see if it passes the tests.
There was a problem hiding this comment.
You are right. I'll check this out and submit a correction shortly.
|
The code should now compile and pass unit tests on both real and complex. |
|
Seems it breaks CUDA build, though. I have no CUDA system readily available to test and fix it, but I'll look into this tomorrow. Until then the changes are best left on hold. Sorry for the inconvenience. |
|
Can you send me the CUDA log? |
In response to issue #1769 from @rcclay, I've added code to compute IonIon forces due to coulomb potential in OBC, when "forces=yes" attribute is present. The value is accumulated with that of ACForce estimator. I've tested that the code works for more than two ions, and checked the validity by hand.