-
Notifications
You must be signed in to change notification settings - Fork 135
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
Cusp Correction for SOA - Serial #1172
Conversation
Can one of the maintainers verify this patch? |
1 similar comment
Can one of the maintainers verify this patch? |
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.
Thanks. I know that this is currently a WIP, but clearly doxygen comments are needed for all these functions. Additional comments throughout would be beneficial.
e.g. What are the magic numbers in getOneIdealLocalEnergy and is it significant that they are only there is 6 s.f.?
@@ -103,8 +103,8 @@ namespace qmcplusplus | |||
} | |||
|
|||
|
|||
LCAOrbitalBuilder::LCAOrbitalBuilder(ParticleSet& els, ParticleSet& ions, Communicate *comm, xmlNodePtr cur) | |||
: SPOSetBuilder(comm), targetPtcl(els), sourcePtcl(ions), myBasisSet(nullptr), h5_path(""), doCuspCorrection(false) | |||
LCAOrbitalBuilder::LCAOrbitalBuilder(ParticleSet& els, ParticleSet& ions, xmlNodePtr cur) |
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.
This is a regression.
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.
Fixed
@@ -516,32 +670,28 @@ namespace qmcplusplus | |||
LCAOrbitalSet *lcos = nullptr; | |||
LCAOrbitalSetWithCorrection *lcwc = nullptr; | |||
if (doCuspCorrection) { | |||
lcwc =new LCAOrbitalSetWithCorrection(sourcePtcl, targetPtcl, myBasisSet, rank()==0); | |||
lcwc =new LCAOrbitalSetWithCorrection(sourcePtcl, targetPtcl, myBasisSet, ReportLevel); |
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.
This is a regression.
In the mixed precision case, we probably need to do the cusp correction construction in full precision while do the evaluation in single. Do we have a plan how to do this? |
Re: mixed-precision. I am also interested in the plan, but for an initial implementation I wouldn't object if we only supported double. A working executable path is needed in SoA to help debug #1145 . |
OK to test |
Keep in Mind that the point is to have this available for now for anyone "stuck with a bug" and also to help find errors if any. I am making the PR because I needed this code, but all the credit goes to @markdewing who made 99.99% of the work. (which means I cannot make the doc for the code) |
I will write some documentation for these functions. |
|
@prckent I am temptd to stage this in 2 separate PRs. one for Serial and one for parallel implementation just because of the urgency of the matter. I am testing the code on a very large set of molecules and in some cases there is significant drop in the variance and multidet is slightly stabilized from large fluctuations. |
@markdewing Do you plan to update the code in this PR with some comments, or should we merge now and update later? |
Also remove some unused code.
I'm working on some comments, documentation, and removing some unused code. I can probably get that into this branch today. |
Could you please move the test/CuspCorrection/CHN to tests/molecules/CHN_ae? |
@prckent
vmc-ref series 0 -93.339342 +/- 0.000215 2.379711 +/- 0.004316 0.0255 Are you refering to something else? (Removed the Walker=1) |
@anbenali Please keep reference input untouched and only change the input file for tests. |
@anbenali Reference error bar for short test can be calculated from the long reference run by rescaling: e.g. if reference run is 100x longer than short, then reference error bar for short test is: 0.000215*sqrt(100+1) = 0.00216. Reason is basically that short runs produce noisy error bars and thus tests fail with actual frequency deviating from 3 sigma. Apologies if this is what you did already, your reference of 0.011 looks like it was taken directly from a single short run. |
@anbenali I also notice that your long reference run uses 200 blocks. This will also create a variable errorbar. For generation of reference error bar many more blocks is desired; use 10000 instead (and fewer steps of course). This will reduce the error bar of the error bar by a lot and also improve estimates of the autocorrelation time. |
For the error bar, the 100x longer or shorter is the wrong way (or at least the unpractical way) to make a test. This requirement would make sens if we have all the time we need... but the hard restriction is not the number of steps but the 30 seconds it needs to run on a given machine... hence putting the error bar raw from the run I used. If I were to use the method you give, the test will always fail since outside the 3sigma... If now I base my ref run on the 30 seconds run then my answer for the reference will have such a large error bar that we would never be able to catch an error... Regarding the reference containing 200 blocks, this was the past requirement to avoid long files in the repo. got my previous PR blocked by @ye-luo for doing what you are suggesting. can we have a consensus here? |
The committed example "ref data" can be short, but the number of blocks actually used to create the reference data and the ref xml file must have more blocks. I usually include scalar.dat files from a run w/ lower blocks (typically 1000) in the repo and add a README file documenting all the reference data (optional?). |
@anbenali phone sometime? Better discussed interactively than through comments here. |
What was the outcome of the discussions? Merge of this PR is officially pending some simple updates currently. We could choose to require them in a later PR (required for the release). |
Anouar is making a new test based on reference data from a 10000 blocks run. IMO we could accept this PR and make an issue relating to the test with another PR anticipated soon. Up to @anbenali. |
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.
Please move the test and connect it.
The current tests are not connected.
Hopefully the changes will please all parties.. |
tests/CMakeLists.txt
Outdated
@@ -43,4 +43,5 @@ else() | |||
SUBDIRS("heg/heg_54_J2rpa") | |||
SUBDIRS("molecules") | |||
SUBDIRS("solids") | |||
SUBDIRS("CuspCorrection") |
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.
No more need this
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.
The short tests are still not stable.
closes #1150