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

Add expanded gaussian pair for hpmc #1817

Merged
merged 34 commits into from
Jul 11, 2024

Conversation

josephburkhart
Copy link
Contributor

@josephburkhart josephburkhart commented Jun 13, 2024

Description

I have created a new expanded gaussian pair potential for HPMC. The implementation of this potential closely follows the implementation of the lennard-jones pair potential for HPMC, which has led to some minor code duplication that may warrant later refactoring. The mathematical definition of the new pair potential should be equivalent to that of the expanded gaussian in the MD module. As part of this change, the following files were created:

  • hoomd/hpmc/PairPotentialExpandedGaussian.h - templated off of PairPotentialLennardJones.h
  • hoomd/hpmc/PairPotentialExpandedGaussian.cc - templated off of PairPotentialLennardJones.cc
  • hoomd/hpmc/pair/expanded_gaussian.py - templated off of lennard_jones.py
  • hoomd/hpmc/pair/pytest/test_pair_expanded_gaussian.py - templated off of test_pair_lennard_jones.py

Motivation and context

Adding this pair potential will allow users to stochastically simulate systems with interactions that are defined by gaussian models, which previously has only been possible in MD. This may provide benefits, for example, in very large systems.

How has this been tested?

  • HOOMD compiled locally without errors
  • ctest completed locally without errors
  • pytest completed locally without errors
  • sphinx documentation built locally without errors and appears correct
  • an example simulation appeared visually correct (tested with both repulsive and attractive parameters)

Change log

Added:

* ``hpmc.pair.ExpandedGaussian`` computes the expanded Gaussian pair potential in HPMC
  (`#1817 <https://github.com/glotzerlab/hoomd-blue/pull/1817>`__).

Checklist:

@josephburkhart
Copy link
Contributor Author

pre-commit.ci autofix

@josephburkhart josephburkhart added enhancement New feature or request hpmc HPMC component labels Jun 13, 2024
@josephburkhart
Copy link
Contributor Author

pre-commit.ci autofix

@josephburkhart josephburkhart marked this pull request as ready for review June 14, 2024 15:59
@josephburkhart josephburkhart requested review from a team, tcmoore3, SchoeniPhlippsn and mariaward10 and removed request for a team June 14, 2024 15:59
@josephburkhart josephburkhart added the validate Execute long running validation tests on pull requests label Jun 18, 2024
@josephburkhart
Copy link
Contributor Author

In an offline conversation, @SchoeniPhlippsn and I were discussing some revisions to the pre-computed values in PairPotentialExpandedGaussian.h, namely, removing the lines that pre-compute r_cut_squared and r_on_squared, since I just take the square root of them in PairPotentialExpandedGaussian.cc.

I pointed out that in PairPotentialExpandedGaussian.cc, there are some calculations that could actually be moved to the header file - many of the lines that calculate shifting/smoothing functions when the mode is xplore or shift do not actually depend on the current value of r at all, so they could just be pre-computed in the header.

I left the code this way because it's how it's structured in the HPMC LJ Pair Potential, and I wanted to keep things consistent. But moving the computation to the header file would make things more efficient. What do others think?

@joaander
Copy link
Member

Yes, you can precompute additional constant terms and store them in the ParamType data structure. This adds memory per type pair, but saves a few cycles per interaction. On the CPU this is probably a performance win.

And this is a good observation on r_cut_squared. It may be best to keep this one consistent with LJ - if we move to a generic base class then they will need to be consistent. Using the pre-computed value avoids the need to compute sqrt(r_cut_squared) for every particle pair interaction when mode == 'shift'.

Copy link
Member

@tcmoore3 tcmoore3 left a comment

Choose a reason for hiding this comment

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

This looks good, nice work.

Regarding whether to precompute quantities or not, I have no strong opinions, so my vote would be to keep as consistent as possible with the other HPMC pair potentials.

1.5, 2.0, 1.0, 1.0),
rel=1e-5)
assert simulation.operations.integrator.pair_energy == pytest.approx(
expected=eg(1.5, 1.0, 1.0, 1.0) + eg(1.5, 2.0, 1.0, 1.0), rel=1e-5)
Copy link
Member

Choose a reason for hiding this comment

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

You could save the expected results into variables. That would help with readability, especially the last line, which becomes

assert simulation.operations.integrator.pair_energy == pytest.approx(expected_1 + expected_2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, I just noticed this feedback. I've just implemented your suggested changes and pushed to this branch.

@joaander
Copy link
Member

joaander commented Jul 2, 2024

We can make an issue and plan to refactor the precomputing in a future PR.

On the GPU, memory is expensive and floating point operations are cheap. This is why the HOOMD MD code does not precompute these quantities. That behavior was copied into HPMC. However, these HPMC potentials are CPU only where the balance is different. I expect a small (but measurable) performance gain by precomputing these on the CPU (especially when sqrt is required).

This is another reason to wait a bit on refactoring common isotropic pair methods into a HPMC base class. We have not yet recognized all of the common elements (e.g. precomputation) that could be combined.

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

Thanks! Merge in trunk-minor and this is done.

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

One more small tweak: we should ensure that sigma is positive as in #1810.

hoomd/hpmc/pair/expanded_gaussian.py Outdated Show resolved Hide resolved
hoomd/hpmc/pair/expanded_gaussian.py Show resolved Hide resolved
@joaander joaander merged commit 2d3c7bf into trunk-minor Jul 11, 2024
34 checks passed
@joaander joaander deleted the Feature/Expanded-Gaussian-For-HPMC branch July 11, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hpmc HPMC component validate Execute long running validation tests on pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants