Refactor grid randomization #4363

markdewing · 2022-12-13T21:08:46Z

Fixes the issue raised in #4362. There will be a lot of CI failures. Not sure the best way to approach the fix and updating the deterministic tests. This is a start to at least introduce a possible fix.

Does some refactoring to put the inputs to the random rotation matrix into their own class. The generation of the rotation matrix is extracted from the randomize_grid function, to make it testable in isolation.

A single unit test is added to the rotation matrix - will need more in the future.

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

Delete the items that do not apply

Bugfix
Refactoring (no functional changes, no api changes)

Does this introduce a breaking change?

Yes

What systems has this change been tested on?

desktop

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.

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
Yes. This PR adds tests to cover any new code, or to catch a bug that is being fixed
N/A. Documentation has been added (if appropriate)

Extract the storage and generation of the random rotation matrix to make it easier to test. Fix the issue in QMCPACK#4362 where the distribution of the rotations does not cover the entire sphere.

PDoakORNL · 2022-12-13T21:12:29Z

src/QMCHamiltonians/RandomRotationState.h

+class RandomRotationState
+{
+public:
+  using RealType   = QMCTraits::RealType;


Delete Type from name, the capitalization already indicates this is a type.

PDoakORNL · 2022-12-13T21:21:45Z

src/QMCHamiltonians/RandomRotationState.h

+  RandomRotationState() : rng_vals_({0.0, 0.0, 0.0}) {}
+  RandomRotationState(RandomGenerator& rng) : rng_vals_({rng(), rng(), rng()}) {}
+
+  TensorType getRandomRotationMatrix() const


I think this should just be a function that takes a RNG and returns a random rotation matrix.

camelto2 · 2022-12-13T21:36:15Z

src/QMCHamiltonians/NonLocalECPComponent.cpp

@@ -866,13 +865,9 @@ void NonLocalECPComponent::evaluateOneBodyOpMatrixdRContribution(ParticleSet& W,
 }

 ///Randomly rotate sgrid_m
-void NonLocalECPComponent::randomize_grid(RandomGenerator& myRNG)
+void NonLocalECPComponent::randomize_grid(const RandomRotationState& rs)


Note that there is also NonLocalECPComponent::randomize_grid(std::vector& sphere, RandomGenerator& myRNG). From what I can tell, this is only used by the legacy NonLocalECPotential_CUDA.cpp. For consistency, should that randomize_grid call be changed as well since the legacy code hasn't been removed yet?

That probably breaks more tests and since we are eventually dropping the legacy, maybe it isn't worth changing. But it seems to me things should be done consistently for whatever is actively in the code base.

Indeed, we will want to fix this everywhere.

The code used by the legacy CUDA code paths will also need to be updated.

There will have to be some discussion around updating the tests. This will be a significant effort even for the smallest possible update and likely need parallelizing over multiple people.

Also make fix in CUDA code.

Add a python script to generate the values (based on comments from the original code)

prckent · 2023-01-04T19:36:07Z

I experimented with an ugly bash-awk-sed combination to update the test values for all CPU build variants as well as for the legacy CUDA variants. It looks like nearly all of the updates can be automated this way. Estimators such as the density will have to be updated by hand. Should I pursue this or is anyone else close to a clean solution?

prckent · 2023-01-05T18:37:35Z

Test this please

prckent · 2023-01-06T16:49:45Z

I have a fully updated set of reference values in #4383 that passes CI. They could be merged independently of this refactor or pulled into this PR.

The conflict in this PR also needs fixing.

What are the preferred next steps? Who has bandwidth?

src/Numerics/RandomRotationMatrix.h

markdewing · 2023-01-06T17:25:30Z

I think #4383 should be merged independently.

Add some documentation to generateRotationMatrix

Keep the parts that don't depend on the RNG in Numerics, change those names to RotationMatrix3D, and put the parts that depend on the RNG in QMCHamiltonians.

…ation

markdewing · 2023-01-07T05:27:28Z

Test this please

src/QMCHamiltonians/NonLocalECPComponent.h

ye-luo · 2023-01-07T16:50:43Z

src/QMCHamiltonians/NonLocalECPotential.cpp

-        PPset[ipp]->randomize_grid(*myRNG);
+      {
+        TensorType rmat = generateRandomRotationMatrix(*myRNG);
+        PPset[ipp]->randomize_grid(rmat);


Could you fuse them into one line and keep the code concise?

src/Numerics/RotationMatrix3D.h

The order of function argument evaluations is undefined in the standard. This leads to a difference in evaluation order between debug and release builds, which causes the order of rng1, rng2, and rng3 to be swapped. In debug builds, the arguments get placed on the stack in the reverse order listed. In release builds, the code is likely inlined, and the arguments effectively get evaluated in the forward order listed.

From random_grid to rotateQuadratureGrid

ye-luo

Is it time to remove the WIP tag?

src/QMCHamiltonians/NonLocalECPotential.cpp

src/QMCHamiltonians/NonLocalECPotential.deriv.cpp

src/QMCHamiltonians/SOECPotential.cpp

tests/solids/bccMo_2x1x1_SOREP/CMakeLists.txt

markdewing · 2023-01-09T01:47:17Z

Test this please

ye-luo

LGTM. Still need non-ANL approval.

… into random_rotation

markdewing · 2023-01-09T03:06:30Z

Test this please

prckent

The python code needs a description at the top saying what it is, what it was used for, where the output went, when it would need to be rerun etc.

Good to see the nearly 100% patch coverage

Also update the script to match a fix for the complex build.

prckent · 2023-01-11T17:36:37Z

Test this pleas

prckent · 2023-01-11T20:06:47Z

Test this please

markdewing added 3 commits December 13, 2022 13:34

Make separate random rotation matrix class

9a90bf9

Extract the storage and generation of the random rotation matrix to make it easier to test. Fix the issue in QMCPACK#4362 where the distribution of the rotations does not cover the entire sphere.

Add unit test for random rotation matrix

ca69969

Removed extra namespace

59f0dc9

PDoakORNL reviewed Dec 13, 2022

View reviewed changes

camelto2 reviewed Dec 13, 2022

View reviewed changes

markdewing and others added 7 commits December 14, 2022 09:46

Move rotation matrix generation to a function

95255ef

Change name to RandomRotationMatrix

af38814

Remove unused code

95c1f84

Move rotation matrix code to Numerics

bd852a9

Also make fix in CUDA code.

Minor fixes to naming and type usage

bc44505

Add more test cases for random rotation matrix

907acfe

Add a python script to generate the values (based on comments from the original code)

Fix complex build

c75f5f8

prckent mentioned this pull request Jan 5, 2023

Updated reference values and integration grid fix #4383

Merged

ye-luo reviewed Jan 6, 2023

View reviewed changes

src/Numerics/RandomRotationMatrix.h Outdated Show resolved Hide resolved

src/Numerics/RandomRotationMatrix.h Outdated Show resolved Hide resolved

markdewing added 4 commits January 6, 2023 23:01

Merge remote-tracking branch 'upstream/develop' into random_rotation

a145259

Change prefix of function from get to generate

a1c3f1c

Add some documentation to generateRotationMatrix

Refactor RandomRotationMatrix

6259abe

Keep the parts that don't depend on the RNG in Numerics, change those names to RotationMatrix3D, and put the parts that depend on the RNG in QMCHamiltonians.

Merge remote-tracking branch 'origin/random_rotation' into random_rot…

33805c0

…ation

Update deterministic tests for grid rng change

d2f8958

ye-luo reviewed Jan 7, 2023

View reviewed changes

markdewing added 4 commits January 8, 2023 14:45

Change function name

5da3b85

From random_grid to rotateQuadratureGrid

Fuse statements onto one line

a43c92d

Minimize include files

d8ccf8d

ye-luo reviewed Jan 8, 2023

View reviewed changes

Remove extra brackets

b947fa3

markdewing changed the title ~~[WIP] Fix grid randomization issue and refactor~~ Refactor grid randomization Jan 8, 2023

Merge branch 'develop' into random_rotation

fca3894

ye-luo previously approved these changes Jan 9, 2023

View reviewed changes

markdewing added 2 commits January 8, 2023 20:47

Fix CUDA build

94a20f4

Merge branch 'random_rotation' of https://github.com/markdewing/qmcpack…

c36d709

… into random_rotation

markdewing dismissed ye-luo’s stale review via c36d709 January 9, 2023 02:48

prckent requested changes Jan 9, 2023

View reviewed changes

Add comments to rotation matrix script

6f30155

Also update the script to match a fix for the complex build.

markdewing mentioned this pull request Jan 10, 2023

Test NLPP parameter derivative #4394

Merged

prckent approved these changes Jan 11, 2023

View reviewed changes

Merge branch 'develop' into random_rotation

02196a9

prckent enabled auto-merge January 11, 2023 17:36

prckent merged commit e49d5ce into QMCPACK:develop Jan 11, 2023

markdewing deleted the random_rotation branch January 11, 2023 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor grid randomization #4363

Refactor grid randomization #4363

markdewing commented Dec 13, 2022

PDoakORNL Dec 13, 2022

PDoakORNL Dec 13, 2022 •

edited

Loading

camelto2 Dec 13, 2022 •

edited

Loading

prckent Dec 14, 2022 •

edited

Loading

prckent commented Jan 4, 2023 •

edited

Loading

prckent commented Jan 5, 2023

prckent commented Jan 6, 2023

markdewing commented Jan 6, 2023

markdewing commented Jan 7, 2023

ye-luo Jan 7, 2023

ye-luo left a comment

markdewing commented Jan 9, 2023

ye-luo left a comment

markdewing commented Jan 9, 2023

prckent left a comment •

edited

Loading

prckent commented Jan 11, 2023

prckent commented Jan 11, 2023

Refactor grid randomization #4363

Refactor grid randomization #4363

Conversation

markdewing commented Dec 13, 2022

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

Does this introduce a breaking change?

What systems has this change been tested on?

Checklist

PDoakORNL Dec 13, 2022

Choose a reason for hiding this comment

PDoakORNL Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

camelto2 Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

prckent Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

prckent commented Jan 4, 2023 • edited Loading

prckent commented Jan 5, 2023

prckent commented Jan 6, 2023

markdewing commented Jan 6, 2023

markdewing commented Jan 7, 2023

ye-luo Jan 7, 2023

Choose a reason for hiding this comment

ye-luo left a comment

Choose a reason for hiding this comment

markdewing commented Jan 9, 2023

ye-luo left a comment

Choose a reason for hiding this comment

markdewing commented Jan 9, 2023

prckent left a comment • edited Loading

Choose a reason for hiding this comment

prckent commented Jan 11, 2023

prckent commented Jan 11, 2023

PDoakORNL Dec 13, 2022 •

edited

Loading

camelto2 Dec 13, 2022 •

edited

Loading

prckent Dec 14, 2022 •

edited

Loading

prckent commented Jan 4, 2023 •

edited

Loading

prckent left a comment •

edited

Loading