Batched driver move abstraction #3762

camelto2 · 2022-01-25T00:57:28Z

Please review the developer documentation
on the wiki of this project that contains help and requirements.

Proposed changes

This PR adds an abstraction layer to the batched drivers for the particle moves. It is intended to keep the VMC/DMC advanceWalkers agnostic about the actual moves that are being done (just particle positions, positions and spins, etc). Right now advanceWalkers is templated on a move type, and uses the templated class MoveAbstraction to handle the specifics of the moves. I am happy to change any names for classes/functions/etc here to make things more clear, I was more concerned with getting something working.

note: Eventually this should be able to handle the work needed for the L2potentials as well

part of #3712, and would allow me to delete #3742

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

Delete the items that do not apply

New feature

Does this introduce a breaking change?

No

What systems has this change been tested on?

macOSX

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

…ove_abstraction

camelto2 · 2022-01-25T16:17:36Z

Need to follow up and add comments to everything

camelto2 · 2022-01-25T17:40:11Z

Need to follow up and add comments to everything

I added documentation detailing the intention of each method in the MoveAbstraction class and describing each member variable. Also updated the docs to include the spin mass in the batched driver sections

ye-luo · 2022-01-25T18:08:19Z

Here is my thoughts

MoveAbstraction constructor should capture ps_dispatcher and walker_elecs. Leave twf_dispatcher out.
Pass twf_dispatcher explicitly in APIs need it.
There should no trace of Crowd object in MoveAbstraction
acceptMove should be wrapped symmetrically to makeMove.

Replace mw_makeMoveWithSpin with mw_makeSpinMove which only deal with spin part. Call flex_makeMove and mw_makeSpinMove in the abstraction.
Add mw_accept_rejectSpinMove. Move the touch of spins from mw_accept_rejectMove to this new function and call it from the wrapper.

still thinking about a better name for MoveAbstraction. MoveManipulator? The object name should be a noun, not "move". Will can do the change once we settle the class name.

ye-luo · 2022-01-25T17:40:15Z

src/QMCDrivers/MoveAbstraction.h

+#ifndef QMCPLUSPLUS_MOVEABSTRACTION_H
+#define QMCPLUSPLUS_MOVEABSTRACTION_H
+
+#include "QMCDriverNew.h"


Move the ENUM in this class. This class doesn't need to depend on QMCDriverNew

PDoakORNL

This PR sets up a new state machine and mixed data and operations class.
Due to the large upfront capture of variables and state members it hides the actual data dependence of many steps of the qmc section which I consider to be undesirable.
The methods are almost all state transformations of the object and not clean functions.

While I feel that breaking up advance walkers into pure functions would be worth while I think templating or giving them if constexpr sections based the Monte Carlo Coordinate type is a much cleaner way to add this functionality.

I would propose working from a different start such as in #3767

ye-luo · 2022-01-25T21:19:26Z

@PDoakORNL can we first complete this one and then work on refactoring? #3767 is a prototype and only partially address what is abstracted in this PR. So it takes a bit more time to work out a fully satisfactory design.

camelto2 · 2022-01-25T21:35:50Z

Here is my thoughts

1. MoveAbstraction constructor should capture ps_dispatcher and walker_elecs. Leave twf_dispatcher out.

2. Pass twf_dispatcher explicitly in APIs need it.

3. There should no trace of Crowd object in MoveAbstraction

This is taken care of with the recent push. Looking at the changes to particleset now.

ye-luo · 2022-01-26T16:36:18Z

src/QMCDrivers/VMC/VMCBatched.cpp

@@ -233,7 +213,13 @@ void VMCBatched::runVMCStep(int crowd_id,
  const bool recompute_this_step = (sft.is_recomputing_block && (step + 1) == max_steps);
  // For VMC we don't call this method for warmup steps.
  const bool accumulate_this_step = true;
-  advanceWalkers(sft, crowd, timers, *context_for_steps[crowd_id], recompute_this_step, accumulate_this_step);
+  const bool spin_move            = crowd.get_walker_elecs()[0].get().isSpinor();


This line can be dangerous if there is no element.
Pass the state via the sft object.
Also use population_.get_golden_electrons()

ye-luo · 2022-01-26T16:39:34Z

src/QMCDrivers/VMC/VMCBatched.cpp

@@ -80,68 +78,37 @@ void VMCBatched::advanceWalkers(const StateForThread& sft,
  std::vector<std::reference_wrapper<TrialWaveFunction>> twf_accept_list, twf_reject_list;
  isAccepted.reserve(num_walkers);

+  MoveAbstraction<COORDS> move(ps_dispatcher, walker_elecs, step_context.get_random_gen(), sft.drift_modifier,


Let us change this to 'mover' a noun.

ye-luo

LGTM. I'm inclined to merge this for fast turnaround and avoid working on forks although I consider MoveAbstraction should go away as we get #3767 ready.

camelto2 · 2022-02-09T23:25:01Z

Made a significant change to this PR. This should be more in line with what @PDoakORNL was thinking.

I completely removed the MoveAbstraction class which this PR introduced.
Instead, I have introduced a new struct TWFGrads, similar to how MCCoords was introduced in #3774, which gets templated on the CoordsType. This allows me to carry around TWFGrads grads_now, grads_new in the advanceWalkers which will carry spatial gradients and spin gradients depending on the CoordsType. This also allowed me to template the TWFdispatcher on CoordsType, so that I can use a templated flex_ API to decide if it is including spin or not for the gradients.
I also templated DriftModifierBase which removes a lot of the divergent code for normal vs. spin calculations in advanceWalkers.

With a little more work, the advanceWalkers will be further simplified so it doesn't have to have any "if constexpr (CT==CoordsType::POS_SPIN)" code

PDoakORNL · 2022-02-15T15:11:32Z

Will start rereview this afternoon.

Seems stale.

PDoakORNL

This looks like a good step. If there is time fix the attribution for TauParams.

PDoakORNL · 2022-01-26T18:04:11Z

src/Particle/ParticleSet.h

@@ -386,6 +385,16 @@ class ParticleSet : public QMCTraits, public OhmmsElementBase, public PtclOnLatt
                                   const std::vector<bool>& isAccepted,
                                   bool forward_mode = true);

+  /** batched version  of acceptMove and reject Move fused, but only for spins
+   *
+   * note: should be called BEFORE mw_accept_rejectMove since the active_ptcl_ gets reset to -1


Thanks for documenting the state requirement.

PDoakORNL · 2022-02-16T19:38:22Z

src/QMCDrivers/TauParams.hpp

+//
+// Copyright (c) 2022 QMCPACK developers.
+//
+// File developed by: Ye Luo, [email protected], Argonne National Laboratory


@ye-luo didn't write this.
Put my name, yours and we have been writing
// Refactored from MCCoords.hpp
instead of File created by.

I think that is more meaningful when you're tearing up or porting a well known file.
The name is better.

I did the rewriting. So I will take care of this.

move switch from TWFdispatcher into TrialWaveFunction

ye-luo · 2022-02-16T21:58:08Z

src/QMCWaveFunctions/tests/test_MO_spinor.cpp

    elec_.mw_makeMove(p_list, iat, displs);
+    elec_.mw_makeSpinMove(p_list, iat, sdispls);


Could update this test to call the new APIs.

yea, I was just working on this

camelto2 · 2022-02-16T23:05:59Z

ok, added a unit test for the TWFGrads class I added. With the current changes, there are only a few spots left in VMCBatched and DMCBatched that need to be abstracted, but those will be handled by a subsequent PR. This one is large enough as it is

ye-luo · 2022-02-16T23:13:03Z

Test this please

ye-luo · 2022-02-16T23:38:00Z

Test this please

camelto2 added 7 commits January 20, 2022 19:46

add abstraction layer in batched qmc

a6d3e61

add spin moves to abstraction

b1c36c0

add selection in VMCBatched

c7a11ff

add advanceWalkers<POSITIONS_SPINS> to vmcbatched

5b8cd85

add DMC move abstraction

b71e250

add advanceWalkers<POSITIONS_SPINS>

39e809a

Merge remote-tracking branch 'upstream/develop' into batched_driver_m…

40dfce7

…ove_abstraction

camelto2 requested a review from ye-luo January 25, 2022 01:02

PDoakORNL self-requested a review January 25, 2022 01:23

camelto2 added 2 commits January 25, 2022 10:32

add comments to MoveAbtraction class

71ef608

add docs for spin mass to batched drivers

f46a9bf

remove unused variable

5169590

ye-luo reviewed Jan 25, 2022

View reviewed changes

move CoordsToMove enum to MoveAbstraction

1f28c8b

PDoakORNL previously requested changes Jan 25, 2022

View reviewed changes

remove TWFdispatcher from MoveAbstraction, add elecs

cd29a55

camelto2 mentioned this pull request Jan 25, 2022

[Proposal] Abstract the Monte Carlo coordinates #3767

Closed

separate mw_ spatial and spin moves in ParticleSet to individual APIs

cdbe395

ye-luo reviewed Jan 26, 2022

View reviewed changes

camelto2 added 3 commits January 26, 2022 10:02

change move to mover in drivers

9132b29

use golden_electrons to get spinor info

b65d664

use golden electrons

7e4e77e

ye-luo previously approved these changes Jan 26, 2022

View reviewed changes

PDoakORNL mentioned this pull request Jan 27, 2022

Monte Carlo Coords Abstraction #3774

Merged

Merge branch 'develop' into batched_driver_move_abstraction

e77f2fe

camelto2 added 2 commits February 9, 2022 16:01

remove MoveAbstraction

618b262

call accept_rejectSpinMove first

2f58150

camelto2 and others added 4 commits February 10, 2022 10:13

fix failing test

877233d

Fix template.

a6a2f15

Add TauParams.hpp

4e5e965

Simplify ContextForSteps

bda3f0b

move switch from TWFdispatcher into TrialWaveFunction

95ae489

PDoakORNL previously approved these changes Feb 16, 2022

View reviewed changes

Merge pull request #1 from camelto2/move_switch_to_TWF

41236e9

move switch from TWFdispatcher into TrialWaveFunction

camelto2 dismissed PDoakORNL’s stale review via 41236e9 February 16, 2022 21:39

Update header info

60abdad

ye-luo reviewed Feb 16, 2022

View reviewed changes

camelto2 added 2 commits February 16, 2022 15:32

update spinor tests to use templated APIs in ParticleSet

c2220ae

add unit test for TWFGrads

22521d2

Merge branch 'develop' into batched_driver_move_abstraction

6593e7a

ye-luo previously approved these changes Feb 16, 2022

View reviewed changes

ye-luo enabled auto-merge February 16, 2022 23:14

add test_TWFGrads to cmake

3c7aeb6

camelto2 dismissed ye-luo’s stale review via 3c7aeb6 February 16, 2022 23:33

ye-luo approved these changes Feb 16, 2022

View reviewed changes

ye-luo merged commit 3f5955e into QMCPACK:develop Feb 17, 2022

camelto2 deleted the batched_driver_move_abstraction branch February 17, 2022 16:24

This was referenced Feb 17, 2022

Commit plan for batched spin-orbit code #3712

Closed

Finish batched driver move abstraction #3867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batched driver move abstraction #3762

Batched driver move abstraction #3762

camelto2 commented Jan 25, 2022 •

edited

Loading

camelto2 commented Jan 25, 2022

camelto2 commented Jan 25, 2022

ye-luo commented Jan 25, 2022

ye-luo Jan 25, 2022

camelto2 Jan 25, 2022

PDoakORNL left a comment

ye-luo commented Jan 25, 2022

camelto2 commented Jan 25, 2022

ye-luo Jan 26, 2022

camelto2 Jan 26, 2022

ye-luo Jan 26, 2022

camelto2 Jan 26, 2022

ye-luo left a comment •

edited

Loading

camelto2 commented Feb 9, 2022

PDoakORNL commented Feb 15, 2022

PDoakORNL left a comment

PDoakORNL Jan 26, 2022

PDoakORNL Feb 16, 2022

ye-luo Feb 16, 2022 •

edited

Loading

ye-luo Feb 16, 2022

camelto2 Feb 16, 2022

camelto2 Feb 16, 2022

camelto2 commented Feb 16, 2022

ye-luo commented Feb 16, 2022

ye-luo commented Feb 16, 2022

		elec_.mw_makeMove(p_list, iat, displs);
		elec_.mw_makeSpinMove(p_list, iat, sdispls);

Batched driver move abstraction #3762

Batched driver move abstraction #3762

Conversation

camelto2 commented Jan 25, 2022 • edited Loading

Proposed changes

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

Does this introduce a breaking change?

What systems has this change been tested on?

Checklist

camelto2 commented Jan 25, 2022

camelto2 commented Jan 25, 2022

ye-luo commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PDoakORNL left a comment

Choose a reason for hiding this comment

ye-luo commented Jan 25, 2022

camelto2 commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ye-luo left a comment • edited Loading

Choose a reason for hiding this comment

camelto2 commented Feb 9, 2022

PDoakORNL commented Feb 15, 2022

PDoakORNL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ye-luo Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelto2 commented Feb 16, 2022

ye-luo commented Feb 16, 2022

ye-luo commented Feb 16, 2022

camelto2 commented Jan 25, 2022 •

edited

Loading

ye-luo left a comment •

edited

Loading

ye-luo Feb 16, 2022 •

edited

Loading