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

Energy density estimator itself #5139

Merged
merged 10 commits into from
Aug 30, 2024

Conversation

PDoakORNL
Copy link
Contributor

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

Proposed changes

Describe what this PR changes and why. If it closes an issue, link to it here
with a supported keyword.

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

Delete the items that do not apply

  • Bugfix
  • New feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Testing changes (e.g. new unit/integration/performance tests)
  • Documentation or build script changes
  • Other (please describe):

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)

@PDoakORNL PDoakORNL force-pushed the energy_density_estimator_itself branch from 1e97473 to 07153b5 Compare August 26, 2024 19:51
@PDoakORNL PDoakORNL marked this pull request as ready for review August 27, 2024 19:44
@PDoakORNL PDoakORNL changed the title [WIP] Energy density estimator itself Energy density estimator itself Aug 27, 2024
@PDoakORNL PDoakORNL requested a review from ye-luo August 28, 2024 14:23
virtual std::size_t getFullDataSize() const { return data_.size(); }
virtual void packData(PooledData<Real>& buffer);
virtual void unpackData(PooledData<Real>& buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document these two functions explicitly buffer->local or local->buffer.

@@ -55,10 +54,12 @@ class PerParticleHamiltonianLogger : public OperatorEstBase

void write(CrowdLogValues& values, const std::vector<long>& walkers_ids);

Real sumOverAll();
Copy link
Contributor

Choose a reason for hiding this comment

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

sumOverAll() const?

int get_block() { return block_; }
private:
bool crowd_clone = false;
PerParticleHamiltonianLogger * const rank_estimator_;
PerParticleHamiltonianLogger* const rank_estimator_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use const OptionalRef<PerParticleHamiltonianLogger>?

@prckent
Copy link
Contributor

prckent commented Aug 29, 2024

General comment on functions such as sumOverAll and packData : this might not yet be in our coding guide but I would much prefer more verbose names to indicate more about what is being summed, packed etc., and as Ye put, in which direction operations are happening. Makes the code much easier to read by requiring less knowledge of the other objects and member data. Not an essential change but we do need to improve accessibility.

@PDoakORNL
Copy link
Contributor Author

The purpose of sumOverAll is shown in test_EnergyDensityEstimator.cpp:140. I expanded the documentation somewhat.

The pack and unpack data will be used in the next PR. It seems that either the MPI communicator would have to travel over the rank scope estimators or they would need deliver either the addresses of their data or a copy of their data to the MPI communicator. I decided that for now a single packed copy with lifetime decoupled from the estimators provided the best compromise between decoupling, performance, and memory use. The documentation for that will be in the next PR since we haven't made to the enclosing scope or level in the call stack where the buffer lives.

@ye-luo
Copy link
Contributor

ye-luo commented Aug 30, 2024

Test this please

@ye-luo ye-luo enabled auto-merge August 30, 2024 19:15
@ye-luo ye-luo merged commit 5cc7ee5 into QMCPACK:develop Aug 30, 2024
38 of 39 checks passed
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.

3 participants