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

[Proposal] Abstract the Monte Carlo coordinates #3767

Closed
wants to merge 4 commits into from

Conversation

PDoakORNL
Copy link
Contributor

Proposed changes

Make an abstraction for the Monte Carlo Coords.

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

Delete the items that do not apply

  • New feature
  • Refactoring (no functional changes, no api changes)

Does this introduce a breaking change?

  • Yes
  • No

What systems has this change been tested on?

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

@ye-luo
Copy link
Contributor

ye-luo commented Jan 25, 2022

I think you are right. The data object needs to be templated on coordinates.
Since you are touching ContextForSteps, it is time to think hard what is the actual role of ContextForSteps.
It ships Deltas and RNG. It seems serving two tasks.
Deltas. I feel it doesn't t need to be persistent and thus not needed to be passed via argument.
RNG. That needs to be passed in.

So If we can can split the role of ContextForSteps, we may not need to make templates from so high up levels like run().

@camelto2
Copy link
Contributor

I think you are right. The data object needs to be templated on coordinates. Since you are touching ContextForSteps, it is time to think hard what is the actual role of ContextForSteps. It ships Deltas and RNG. It seems serving two tasks. Deltas. I feel it doesn't t need to be persistent and thus not needed to be passed via argument. RNG. That needs to be passed in.

in the approach used in #3762, all of the deltas are handled in the MoveAbstraction class and could be removed from ContextForSteps

@PDoakORNL
Copy link
Contributor Author

The mistake in ContextForSteps is that the deltas are state. They should have just been returned by pure function not in ContextForSteps.

template<MCCoordTypes MCT, class RNG>
MCCoords<MCT> generateDeltas(RNG rng, size_t num_rs)
{
   MCCoords<MCT> mc_coords;
   mc_coords.rs.resize(num_rs);
   makeGaussRandomWithEngine(mc_coords.rs, rng);
   if constexpr (std::is_same<decltype(walker_deltas_), MCCoords<MCCoordsTypes::RSSPINS>>::value)
  {
      mc_coords.spins.resize(num_rs);
      makeGaussRandomWithEngine(mc_coords.spins, rng); 
  }
  return mc_coords;
}

@PDoakORNL
Copy link
Contributor Author

PDoakORNL commented Jan 26, 2022

@ye-luo It wasn't too hard reverse the bad delta_r in ContextForSteps decisions and that indeed makes it easy to push down the templating.

As its clear from @camelto2 #3762 there is some shared code from DMC/VMC Batched.

But I don't think they need to be collected into a stateful black box. The abstraction could be a series of templated pure functions and appropriately constructed SoA data types.

void getDrifts(RealType tau, const std::vector<GradType>& qf, std::vector<PosType>&) const final;

void getDrifts(RealType tau, const std::vector<GradType>& qf, MCCoords<MCCoordsTypes::RSSPINS>&) const final;
void getDrifts(RealType tau, const std::vector<GradType>& qf, MCCoords<MCCoordsTypes::RS>&) const final;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this pattern much more than conexpr if. Namely, API becomes reactive. Cody has refactored the ParticleSet APIs
mw_makeMoveWithSpin has been replaced with mw_makeSpinMove.
The old mw_makeMoveWithSpin can be achieved with mw_makeMove and mw_makeSpinMove.
Then I think we can introduce

makeMove(MCCoords<MCCoordsTypes::RSSPINS>&)
makeMove(MCCoords<MCCoordsTypes::RS>&)

on top of existing APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd much prefer a templated function but because of the unecessary virtual inheritance in the DriftModifiers we are stuck with the overloading.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you want is a templates function with specialization without the class being templated. We have to go for CRTP I guess. Then do we really want the whole class being templated? Since we anyway need to specialize the function. Just putting distinct functions is totally fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in many cases we don't want the whole class templated. If we do maybe CRTP is useful maybe we actually really gain something from the virtual inheritance.

But drift modifier would be better without inheritance. I think this a case that clearly would be better written with if constrexpr and no vtable. The overloads are going to cost several vtable calls without repeating code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean a ParticleSet::makeMove overload set?
That I think makes sense.
Hiding the QMC algorithm in another state machine I'm not in favor of.

Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the direction. It is the right thing we have to explore.
My suggestions is we merge #3762, then gradually the COORD abstraction in ParticleSet and dissolve MoveAbstraction.

Whether to start from a fresh branch or rebase. It will be your choice.

for (int iat = start_index; iat < end_index; ++iat)
{
auto delta_r_start = it_delta_r + iat * num_walkers;
auto delta_r_end = delta_r_start + num_walkers;
auto delta_start = delta_offset + iat * num_walkers;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to avoid these intermediate _start _end. You know how much I dislike iterators.

@@ -344,7 +368,11 @@ void DMCBatched::runDMCStep(int crowd_id,
// Are we entering the the last step of a block to recompute at?
const bool recompute_this_step = (sft.is_recomputing_block && (step + 1) == max_steps);
const bool accumulate_this_step = true;
advanceWalkers(sft, crowd, timers, dmc_timers, *context_for_steps[crowd_id], recompute_this_step,
if(sft.population.get_golden_electrons()->isSpinor())
advanceWalkers<MCCoords<MCCoordsTypes::RSSPINS>>(sft, crowd, timers, dmc_timers, *context_for_steps[crowd_id], recompute_this_step,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to have advanceWalkers<MCCoordsTypes::RSSPIN>

RealType oneover2tau = 0.5 / (tauovermass);
RealType sqrttau = std::sqrt(tauovermass);
auto createTaus = [&](int ig) -> Taus<RealType, MCCOORDS::mct> {
if constexpr (std::is_same<MCCOORDS, MCCoords<MCCoordsTypes::RSSPINS>>::value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to make it abstracted. Doesn't like if constexpr

int start_index = step_context.getPtclGroupStart(ig);
int end_index = step_context.getPtclGroupEnd(ig);
int start_index = step_context.getPtclGroupStart(ig);
int end_index = step_context.getPtclGroupEnd(ig);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to ask Crowd leader instead of step_context.

@@ -83,60 +84,83 @@ void VMCBatched::advanceWalkers(const StateForThread& sft,
for (int sub_step = 0; sub_step < sft.qmcdrv_input.get_sub_steps(); sub_step++)
{
//This generates an entire steps worth of deltas.
step_context.nextDeltaRs(num_walkers * sft.population.get_num_particles());
std::size_t num_deltas = num_walkers * sft.population.get_num_particles();
auto deltas = generateDeltas<MCCOORDS>(step_context.get_random_gen(), num_deltas);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the style but I prefer this object allocation persist across substeps.

MCCOORD::generate(deltas, step_context.get_random_gen())

template<MCCoordsTypes MCT>
struct MCCoords
{
static constexpr MCCoordsTypes mct = MCT;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this indirection can be removed

struct MCCoords;

template<MCCoordsTypes MCT>
struct MCCoords
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We need this class.

};

template<typename Real, MCCoordsTypes CT = MCCoordsTypes::RS>
struct Taus
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this class.



template<class MCC, class RNG>
MCC generateDeltas(RNG rng, size_t num_rs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us move this class to src/Particle/ParticleBase/RandomSeqGenerator.h
Also move this file to src/Particle

namespace qmcplusplus
{

enum class MCCoordsTypes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. we need this enum. CoordsTypes no need of MC.
Prefer longer names. So use POS and POS_SPIN?

@PDoakORNL
Copy link
Contributor Author

This has served it purpose and a useful subset submitted as #3774

@PDoakORNL PDoakORNL closed this Jan 27, 2022
@PDoakORNL PDoakORNL deleted the abstract_MCCoords branch February 23, 2022 00:15
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.

None yet

3 participants