-
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
Add param_array to CPPExternalPotential #1608
Conversation
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
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.
Thanks! Changes are needed to not break the existing API and expand testing.
@@ -10,17 +10,13 @@ | |||
# check if llvm_enabled | |||
llvm_disabled = not hoomd.version.llvm_enabled | |||
|
|||
valid_constructor_args = [dict(code='return -1;')] | |||
valid_constructor_args = [dict(code='return -1;', param_array=[1])] |
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.
Also test with no param_arary
to ensure that the existing API functions.
@@ -175,7 +198,8 @@ def test_electric_field(device, orientations, charge, result, | |||
|
|||
sim = simulation_factory(two_particle_snapshot_factory()) | |||
|
|||
ext = hoomd.hpmc.external.user.CPPExternalPotential(code=electric_field) | |||
ext = hoomd.hpmc.external.user.CPPExternalPotential(code=electric_field, | |||
param_array=[0]) |
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 code doesn't use param_array
. It should remain unchanged and should continue to function.
hoomd/hpmc/external/user.py
Outdated
@@ -76,7 +85,7 @@ class CPPExternalPotential(ExternalField): | |||
|
|||
gravity_code = "return r_i.z + box.getL().z/2;" | |||
gravity = hoomd.hpmc.external.user.CPPExternalPotential( | |||
code=gravity_code) | |||
code=gravity_code, param_array=[]) |
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.
code=gravity_code, param_array=[]) | |
code=gravity_code) |
The user should not need to explicitly set param_array=[]
. Doing so will break all existing scripts that use `CPPExternalPotential).
Also, update this example to run successfully in Sybil.
Co-authored-by: Joshua A. Anderson <joaander@umich.edu>
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.
Looks good to me, thanks for adding this functionality.
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.
Thanks!
Description
Adds variable parameters to
hpmc.external.user.CPPExternalPotential
, directly analogous to theparam_array
s in thehpmc.pair.user
classes.This is technically an API-breaking change suited for a major release, but there is a clear warning in the python class documentation that this feature is experimental.
Motivation and context
Can change parameters in the external potential function.
How has this been tested?
The existing tests pass, and I have been using it in exploratory simulations for research. I haven't yet, but I will add a more direct unit test of changing the values in
param_array
.Change log
Checklist:
sphinx-doc/credits.rst
) in the pull request source branch.