Skip to content
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

[WIP] Fix for Issue #2330 #2349

Closed
wants to merge 3 commits into from
Closed

Conversation

mcbennet
Copy link
Contributor

Proposed changes

This PR contains a proposed fix to Issue #2330.
Further testing is needed to understand what builds
continue to show bias and why.

What type(s) of changes does this code introduce?

  • Bugfix

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

Workstation and NERSC Cori

Checklist

  • Yes. This PR is up to date with current the current state of 'develop'
  • Yes. Code added or changed in the PR has been clang-formatted
  • No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
  • No. Documentation has been added (if appropriate)

This PR contains a proposed fix to Issue QMCPACK#2330.
Further testing is needed to understand what builds
continue to show bias and why.
@qmc-robot
Copy link

Can one of the admins verify this patch?

@PDoakORNL
Copy link
Contributor

The whole resetRun thing for SimpleFixedNodeBranch was added in
a24d9a7
as a "bug fix" 😭

It was called in DMC::resetComponents(xmlNodePtr cur)
I deleted it in 982ac99 when I tried to kill reuse of QMCDriver subtypes and removed the call to resetComponents for DMC.
Looking at whether any other damage might have been done to the legacy driver there.
Or if any state manipulation was dropped there. I don't think so since I try to bring just the "state" hiding in SFNB into the unified driver.

For DMC legacy CUDA it is still called via another code path.

@mcbennet
Copy link
Contributor Author

@PDoakORNL something I have noticed that might be helpful as you look around is that when I run a deterministic test of the form VMC->DMC_timestep1->DMC_timestep2, and compare to 3.8.0, I see that all VMC quantities match to all decimals in the scalar files but only AoS gives matching quantities for DMC_timestep1 (while SoA does not give matching quantities).

After combing through my outputs, I noticed that for AoS, DMC_timestep1 starts with the same trial energy between develop and 3.8.0 while SoA produces two different starting trial energies for 3.8.0 and develop.

@PDoakORNL
Copy link
Contributor

Looking at QMCDriver.cpp since the driver fishes the previous drivers EstimatorManagerBase point out of the SFNB Estimators->reset() needs to be called there since that was in resetComponents as well.

@PDoakORNL
Copy link
Contributor

@mcbennet Apologies for pushing directly to your branch. I intended to push to a branch on my fork but was a little to fast on the keyboard.

@mcbennet mcbennet closed this Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants