mosdef-hub / foyer

A package for atom-typing as well as applying and disseminating forcefields
https://foyer.mosdef.org
MIT License
118 stars 77 forks source link

`Forcefield.get_parameters` returns empty lists for periodic torsions in the GAFF Forcefield #470

Closed umesh-timalsina closed 2 years ago

umesh-timalsina commented 2 years ago

Bug summary

With GAFF forcefield in Foyer, the following parameters are not returned

https://github.com/rsdefever/GAFF-foyer/blob/1108edf84e436ff5a0cec826d4d1371b837b2965/gafffoyer/xml/gaff.xml#L6437

Code to reproduce the behavior

Please include a code snippet that can be used to reproduce this bug.

>>> from foyer import forcefields
Warning: importing 'simtk.openmm' is deprecated.  Import 'openmm' instead.
>>> gaff = forcefields.load_GAFF()
/home/umesh/miniconda3/envs/forcefields-dev/lib/python3.9/site-packages/foyer/forcefield.py:620: UserWarning: No force field version number found in force field XML file.
  warnings.warn(
/home/umesh/miniconda3/envs/forcefields-dev/lib/python3.9/site-packages/foyer/forcefield.py:632: UserWarning: No force field name found in force field XML file.
  warnings.warn(
/home/umesh/miniconda3/envs/forcefields-dev/lib/python3.9/site-packages/foyer/forcefield.py:644: UserWarning: No combining rule found in force field XML file.
  warnings.warn(
/home/umesh/miniconda3/envs/forcefields-dev/lib/python3.9/site-packages/foyer/validator.py:165: ValidationWarning: You have empty smart definition(s)
  warn("You have empty smart definition(s)", ValidationWarning)
>>> ff_params = gaff.get_parameters(group="periodic_propers", key=['', 'c', 'c1', ''], keys_are_atom_classes=True)
>>> ff_params
{'periodicity': [], 'phase': [], 'k': []}
>>> 

Software versions

umesh-timalsina commented 2 years ago

Looking at the OpenMM code base https://github.com/openmm/openmm/blob/610e92fa5f79fbc8cc48ea96d221b0583f817839/wrappers/python/openmm/app/forcefield.py#L2219-L2226

and thanks to @justinGilmer the PeriodicTorsion force is almost always zero when the parameter k=0, which means its not necessary to apply it while creating an OpenMMSystem, but since function ForceField.get_parameters is not applying those parameters to the function but only returning them, I think we should return them as is (even if k == 0).