Port spin density observable #2840
Conversation
| sdi.readXML(node); | ||
| SpeciesSet species_set; | ||
| int ispecies = species_set.addSpecies("C"); | ||
| int iattribute = species_set.addAttribute("membersize"); |
There was a problem hiding this comment.
What is the version of clang-format you used? This issue should have been fixed.
There was a problem hiding this comment.
7 was used.
Then I tried version 7,10,12 (develop)
|
Please send me an email with your input and output files. I will make a separate PR to address the needs of qdens. |
|
How are you testing the correctness of the new implementation? |
PDoakORNL
force-pushed
the
port_spin_density_observable
branch
from
aa1b378
to
cff2988
Compare
There was a problem hiding this comment.
I think this is ready to go. Short of starting over and doing a minimal change version still attached to QMCHamiltonian and Collectables there is only the skeleton of the datalocality handling which I'd like to stay there since I'm going to immediately be using them for the density matrices and the huge spindenisty implementation.
|
As far as testing that this produces the same result as the original out side of running the full app and comparing a statistical result it is not easy since SpinDensity has absolutely no unit testing and depends on the assumption of a tight coupling to a single walker state machine as is the norm for the estimators. What should be trivial, accumulating once and checking the data structure is quite painful. But I expect I will have to write a couple of unit tests for the original to be sure. Outside of that a statistical test with the full app seems like all that can be done. This PR is just an iterative step so I can stop rebasing against develop and review is not delayed by to much code to review. |
PDoakORNL
force-pushed
the
port_spin_density_observable
branch
from
9e27765
to
5b948ad
Compare
PDoakORNL
force-pushed
the
port_spin_density_observable
branch
from
5b948ad
to
e300937
Compare
|
Are there any outstanding questions about this PR? |
|
@jtkrogel Does this PR look good to you? |
|
(I understand this PR is a step along the way, so provided there are no major objections on the direction, I hope we can merge.) |
Proposed changes
This is a port of the legacy SpinDensity estimator from the legacy architecture. It is the beginning of the strip down refactor of OperatorBase as well. SpinDensityNew derives from OperatorEstBase which manages a generic buffer of QMCTraits::RealType for the derived OperatorEstimators. This replaces the use of Collectables as the data store for these estimators.
This sort of estimator is now instantiated under the "control" of EstimatorManagerNew and is accumulated when other estimators are accumulated not when the Hamiltonian is evaluated.
Right now each SpinDensity new gets its own copy of the accumulator data structure, a pending PR will address the issue of large SpinDensities.
MPI reduction occurs between ranks 1 time per estimator per block for SpinDensity.
I will provide additional documention of the new estimator design soon. I'm beginning work on the DensityMatrix estimator now. But this is as much as I think should be reviewed at once.
@jtkrogel Right now qdens will not work with the hdf5 output because (as far as I can tell) it needs to be able to understand the input.xml which it does not. I tried to modify nexus to allow this but there is something I'm not grasping about qmcpack_input.py and qdens.
What type(s) of changes does this code introduce?
port of SpinDensity from QMCHamiltonian based legacy driver architecture to unified batched driver architecture and management by EstimatorManagerNew and not QMCHamiltonian.
Does this introduce a breaking change?
What systems has this change been tested on?
Leconte
Checklist