fix Madelung constant in CoulombPBCAA #3806
Conversation
|
Thanks Paul. Some background could be helpful for further improvements: how did you notice this? Could the other Coulomb terms (AB) also have the same issue? |
|
@prckent I need to add the Madelung term for AFQMC total energy. As Jaron noted, this term affects only the output. As far as I can tell, |
src/QMCHamiltonians/CoulombPBCAA.cpp
Outdated
| app_log() << " PBCAA total constant " << Consts << std::endl; | ||
| //app_log() << " MC0 of PBCAA " << MC0 << std::endl; | ||
| } |
| @@ -27,6 +27,7 @@ namespace qmcplusplus | |||
| { | |||
| TEST_CASE("Coulomb PBC A-A", "[hamiltonian]") | |||
| { | |||
| double vmad_sc = -1.4186487397403098; | |||
There was a problem hiding this comment.
Use const to avoid unintended altering of values.
| //cout << "val = " << val << std::endl; | ||
| REQUIRE(val == Approx(-1.418648723)); // not validated | ||
| //std::cout << "val = " << val << std::endl; | ||
| REQUIRE(val == Approx(vmad_sc)); |
There was a problem hiding this comment.
REQUIRE stops the execution. So better suited for flow control or nullptr checks.
CHECK are more suitable for value checks. So please use CHECK.
| } | ||
|
|
||
| TEST_CASE("Coulomb PBC A-A BCC H", "[hamiltonian]") | ||
| { | ||
| double alat = 3.77945227; |
| @@ -101,8 +108,12 @@ TEST_CASE("Coulomb PBC A-A BCC H", "[hamiltonian]") | |||
| REQUIRE(consts == Approx(-1.675229452)); // not validated | |||
|
|
|||
| double val = caa.evaluate(elec); | |||
| //cout << "BCC H val = " << val << std::endl; | |||
| //std::cout << "BCC H val = " << val << std::endl; | |||
| elec.create(1); | ||
| elec.R[0][0] = 0.0; | ||
| elec.R[0][1] = 0.0; | ||
| elec.R[0][2] = 0.0; |
There was a problem hiding this comment.
You can write
elec.R[0] = {0.0, 0.0, 0.0}
Let's make the code more concise.
| int chargeIdx = tspecies.addAttribute("charge"); | ||
| int massIdx = tspecies.addAttribute("mass"); | ||
| int pMembersizeIdx = tspecies.addAttribute("membersize"); | ||
| tspecies(pMembersizeIdx, upIdx) = 1; |
|
@ye-luo I have addressed all your comments. |
|
Test this please |
Please review the developer documentation
on the wiki of this project that contains help and requirements.
Proposed changes
Add the missing background term (
vs_k0) into the Madelung constant.This makes it correct for simple cubic and body center cubic crystals as shown by the unit tests.
What type(s) of changes does this code introduce?
Does this introduce a breaking change?
What systems has this change been tested on?
workstation
Checklist
Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.