-
Notifications
You must be signed in to change notification settings - Fork 127
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
External potentials in HPMC. #1811
Conversation
Implement ExternalPotential. Pass trial=true/false to control whether walls will evaluate old energies or assume 0. The old API is not possible to integrate with the new - callers will need to use both for now. Implement the new API in IntegratorHPMCMono.h. TODO: - [ ] Implement in BoxMC and MuVT. - [ ] Implement total energy evaluation (ext.energy and mc.external_energy). - [ ] Python API. - [ ] Gravity potential.
* Use computeTotalExternalEnergy in BoxMC. * Add `mc.external_energy` TODO: - [ ] Implement external in muVT - [ ] Python API. - [ ] Gravity potential.
TODO: - [ ] Python API. - [ ] Gravity potential.
I did not get very far because I ran into errors in my new test that I could not figure out how to fix. I wrote a test that tests attaching external potentials to the simulation. However, an exception is raised when HOOMD tries to attach the linear external potential. The error message I get is ``` TypeError: (): incompatible function arguments. The following argument types are supported: 1. (arg0: hoomd.hpmc._hpmc.ExternalPotentialLinear, arg1: hoomd._hoomd.vec3_double) -> None Invoked with: <hoomd.hpmc._hpmc.ExternalPotentialLinear object at 0x10706f3f0>, (0.0, 0.0, 0.0) ``` The 2nd argument in the "invoked with:" list is a `_HOOMDTuple` and not a `vec3_double`. I'm not sure if that's the cause of the error, but it's my best guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be in good shape, I just had two small comments.
vec3<Scalar> origin(m_pdata->getOrigin()); | ||
const BoxDim box = this->m_pdata->getGlobalBox(); | ||
vec3<Scalar> r0 = m_reference_positions[h_tags.data[index]]; | ||
Scalar3 t = vec_to_scalar3(position - origin); | ||
box.wrap(t, dummy); | ||
vec3<Scalar> shifted_pos(t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did this never trigger an unused variable warning?
old_mean_z = np.mean(snapshot.particles.position[:, 2]) | ||
old_energy = ext.energy | ||
|
||
for n in range(10): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the bug this test uncovered, it's a good thing you happened to realize we could adopt the test from CPPExternalPotential
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if our gravity with no walls test would look more reasonable now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if our gravity with no walls test would look more reasonable now?
It won't, we ran than in serial...
Also move the slightly longer running z_bias test to validate.
Description
Motivation and context
Replace
CPPExternalPotential
.How has this been tested?
Added unit tests.
Change log
Checklist:
sphinx-doc/credits.rst
) in the pull request source branch.