-
Notifications
You must be signed in to change notification settings - Fork 33
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
Charmm parameters writer #848
base: main
Are you sure you want to change the base?
Conversation
|
||
Parmed stores rmin/2 in "rmin" | ||
""" | ||
unit_system = LAMMPS_UnitSystems("real") # ang, kcal/mol, amu |
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.
Do we need to make sure we're using this everywhere we write out parameters? I see we use it for the atom mass, but not anywhere else.
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.
Yep, for in order to handle any input unyts this will do the filtering. I was just too lazy to do it every time at first. Was wondering if you think we should make a system that isn't specific to LAMMPS for this one. It is the exact same as the real units, but maybe a touch confusing to copy them across to other engines. What do you think?
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 unit_system
should be a parameter of write_prm
that defaults to None
. If nothing is passed, then all units are used as-is from the toplogy? If we set unit_system = topology.unit_system
as default behavior, would that work as well?
def write_prm(topology, filename, unit_system=None):
if not unit_system:
unit_system = topology.unit_system
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.
The PRM file looks to me like it has default assumed units of kCal/mol, angstrom, amu, radians, etc for it's units. So it can only convert to one defined system.
gmso/formats/prm_writer.py
Outdated
) | ||
|
||
f.write("\nNONBONDED\n") | ||
nonbonded14 = topology.scaling_factors[0][2] |
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 doesn't need to be fixed here, but I feel like this attribute should be more readable rather than just knowing that 0th index is LJ and 1st index is electrostatics. Maybe a dict? I can open a separate issue.
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.
Was thinking the same thing myself. I think it should also be {"electrostatics":{"1-4":0.5}}, instead of a fixed list. Could potentially be some odd 1-5 interactions at some point.
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.
Yeah, that's sort of what I was thinking as well.
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 LGTM. It seems like we're just missing some type checks to ensure only supported potential formats are being used?
Cool. Thanks @chrisjonesBSU, yeah I'll punch those in this week to get this writer off the ground. I'm not sure it gets a lot of use, but it'll be nice to have. |
for more information, see https://pre-commit.ci
# if key not in uniqueTypesDict: | ||
# sigma | ||
# epsilon | ||
# name | ||
# mass |
Check notice
Code scanning / CodeQL
Commented-out code Note
# else: | ||
# vals = uniqueTypesDict[key] |
Check notice
Code scanning / CodeQL
Commented-out code Note
Port over the old CHARMM/NAMD .prm file writer from mBuild to GMSO. Adding to the savers and allowing for basic charmm style potentials to be written only.
TODOS: