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 additional potential templates #233

Merged
merged 1 commit into from
Feb 15, 2020

Conversation

rsdefever
Copy link
Member

@rsdefever rsdefever commented Feb 14, 2020

Adding four more potential templates for potentials that are supported in Cassandra.

  1. Mie non-bonded potential
  2. Harmonic torsion
  3. OPLS-style fourier series torsion
  4. Harmonic improper

I didn't see any unit tests for the other potentials defined in lib, but please let me know what tests I should add if we would like some.

One possible issue I see is the HarmonicImproperPotential has the same functional form and parameters as the HarmonicDihedralPotential. I think those will evaluate to equivalent in _check_single_potential, #121 .

Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

LTGM, thanks!

  • We do not have unit tests for these, and I'm not sure what shape they would take. Import, make sure things look the way they're expected to? Meh. I'd rather let these be tested as we develop engine compatibility.
  • I'm not entirely sure how to treat the m/n values in the Mie potential. They're not parameters in the sense that they are tunable coefficients that hold physical units (and some amount of physical meaning). I would think that two Mie potentials with different values should be treated as different for purposes of matching at the level of engine compatibility. It think that would imply this class would take in m/n values in the __init__ and populate the SymPy expression with those values. Although that would make engine compatibility checking tricky; one wouldn't be able to just import the template and compare against the expressions in a topology's atom type set. Not sure how to deal with this

@rsdefever
Copy link
Member Author

I think that if an engine supports a Mie potential then it works for any arbitrary value of m and n. This is the case for Cassandra and LAMMPS (https://lammps.sandia.gov/doc/pair_mie.html). Unless I am missing something this can be treated the same as any other potential.

Copy link
Collaborator

@justinGilmer justinGilmer left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks ryan!!

@ahy3nz
Copy link
Collaborator

ahy3nz commented Feb 15, 2020

#31

We have some running issues to keep track of the functional forms supported, and how each engine treats these. If we're expanding, this might help to update

@mattwthompson
Copy link
Member

My concern with parsing the Mie potential had more to do with where the values of m and n are stored; if a Topology object has an atom type with a particular values of m and n stored in the expression, then it will fail all comparisons with this template. This will become more clear when we work on the writers, and I think it may be best to deal with that then so I'll go ahead and merge this now. Thanks for the contribution!

@mattwthompson mattwthompson merged commit cc3d237 into mosdef-hub:master Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants