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

Have a way to set the pppm kwargs #55

Closed
chrisjonesBSU opened this issue Sep 19, 2023 · 1 comment · Fixed by #61
Closed

Have a way to set the pppm kwargs #55

chrisjonesBSU opened this issue Sep 19, 2023 · 1 comment · Fixed by #61
Assignees

Comments

@chrisjonesBSU
Copy link
Member

We currently don't have a way for the pppm parameters to be set for the long and short range charge interactions. We could add this as a prameter to System but I think it would be better if its available/editable in the Simulation class. First we need to determine if this is something that can be modified once the hoomd forces are created.

@chrisjonesBSU
Copy link
Member Author

chrisjonesBSU commented Sep 22, 2023

The order and resolution parameters can be changed as long as the simulation hasn't run, but there is this warning message in the hoomd docs:

make_pppm_coulomb_forces sets all parameters for the returned Force objects given the input resolution and order. Do not change the parameters of the returned objects directly.

So, I don't think changing these after the fact will be a good idea, if the other parameters determined from them aren't updated.

We could add parameters for these that are passed along to GMSO so they are set correctly initially. However, we're kind of running into the issue of having all these parameters in System.__init__ that are only ever used by _apply_forcefield. This isn't a great workflow IMO, especially since System can be used without a forcefield

I think we should revisit the idea of making _apply_forcefield public, and moving these parameters there. This requires the user to call it and define all of these parameters there instead of in System.__init__

@chrisjonesBSU chrisjonesBSU linked a pull request Sep 25, 2023 that will close this issue
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 a pull request may close this issue.

3 participants