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

Safer distance table implementation #2177

Merged
merged 33 commits into from
Jan 3, 2020
Merged

Conversation

ye-luo
Copy link
Contributor

@ye-luo ye-luo commented Jan 1, 2020

This PR focuses on ParticleSet and DistanceTableData implementation.
Changes are:

  1. Mark most DistanceTableData members with protected and public. All the members belonging to SoA are converged.
  2. Remove ParticleSet::setActive. setActive prepare old distances and displacements before a particle is moved. This is part of a state machine and only safe to use in certain scenarios because setActive may overwrite extra memory. After examining its use, its role can be moved inside the makeMove function. When a move is proposed, making sure the old value is updated in the right place. If a DT consumer needs the old value before the call to propose a move, it should request full table availability via addTable.
  3. ParticleSet::makeMoveXXX functions support a maybe_accept flag. If the caller knows the fact that no move will be accepted. Setting maybe_accept = false can save a lot of computation. For example in NLPP.
  4. Make ParticleSet::acceptMove safe. The old acceptMove was safe only if move of all the particles are proposed once and in order. This is a rather unsafe assumption. In this PR, acceptMove are made always maintaining up-to-date distance tables. Only in specific cases, forward = true optimization can be requested for example during p-by-p random walking. In the NLPP force evaluation, update() full table is no more needed after an acceptMove.

Overall, less safe codepath for optimization are more tightly controlled. It allows to write working but non-optimal code easier because state machines are less exposed.

Since the base class DistanceTableData is more of an abstraction and actual data are moved to derived class. DistanceTable is a more appropriate name. I will do renaming in a separate PR.

@markdewing
Copy link
Contributor

The name 'forward' for the flag seems like it could be more descriptive. For me it invokes images of mail forwarding or C++ move operations.

Thinking through the flag, what it does, and when it is used:
It is used when an algorithm moves through the particle indices in a particular sequence. This enables an optimization to reduce the amount of work to be done where the table only needs to be partially updated at each step.

Perhaps 'partial_update' or 'partial_table_update'?

Copy link
Contributor

@PDoakORNL PDoakORNL left a comment

Choose a reason for hiding this comment

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

Overall change is a step forward. There are a few typos to fix and some missed opportunities to modernize/clarify naming. Some of these could be address in a future PR.

src/OhmmsPETE/TinyVector.h Outdated Show resolved Hide resolved
src/Particle/AsymmetricDistanceTableData.h Show resolved Hide resolved
using IndexType = QMCTraits::IndexType;
using RealType = QMCTraits::RealType;
using PosType = QMCTraits::PosType;
using DistRowType = Vector<RealType, aligned_allocator<RealType>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Type does not seem needed, DistRow with the type formatting sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed Type suffix.

src/Particle/DistanceTableData.h Show resolved Hide resolved

/** Displacements[N_targets]x[3][N_sources]
* Note: This is a memory view using the memory space allocated in memoryPool
* Note: Derived classes decide if it is a memory view or the actaully storage
Copy link
Contributor

Choose a reason for hiding this comment

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

actually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

int Ntargets_padded;

///actual memory for Displacements
aligned_vector<RealType> memoryPool_displs_;
Copy link
Contributor

Choose a reason for hiding this comment

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

strangely named method or variable? memory_pool_displs_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

* @param P the target particle set
* @param rnew proposed new position
* @param iat the particle to be moved
* @param prepare_old if true, prepare old distances and displacements for using getOldDists and getOldDispls functions later.
Copy link
Contributor

Choose a reason for hiding this comment

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

hopefully temporary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old values should be temporarily used only.

src/QMCHamiltonians/LocalECPotential.h Show resolved Hide resolved

/** return the temporary distances when a move is proposed
*/
const DistRowType& getTemporalDists() const { return Temp_r; }
Copy link
Contributor

Choose a reason for hiding this comment

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

getTempDists() is clearer to me. Temporal is not a common synonym of temporary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All changed.

src/Sandbox/diff_distancetables.cpp Show resolved Hide resolved
@ye-luo
Copy link
Contributor Author

ye-luo commented Jan 3, 2020

@markdewing I changed DTD forward to partial_update and ParticleSet forward to partial_table_update as you suggested.

@prckent
Copy link
Contributor

prckent commented Jan 3, 2020

This is moving in a good direction.

It is clear that the design is still not right - it is too complex still, although this is definitely an improvement. Perhaps indicating where state is involved in the function names might help.

@prckent prckent merged commit 9f7ad03 into QMCPACK:develop Jan 3, 2020
@ye-luo ye-luo deleted the safer-DT branch January 3, 2020 23:23
@ye-luo ye-luo mentioned this pull request Apr 12, 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.

4 participants