mosdef-hub / foyer

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

Fix scaling factors and mixing rule bug when atomtype with `general_forcefield` #489

Closed daico007 closed 2 years ago

daico007 commented 2 years ago

PR Summary:

Currently, when perform atomtyping with general_forcefield.Forcefield, the combining_rule and scaling_factors are not being transferred correctly. This PR makes sure the scaling_factors is passed correctly to the final gmso.Topology. The combining_rule can be specified during the Forcefield.apply() called, but I just realized that the combining_rule is not an attribute of GMSO Forcefield, so that will be implemented in a separate PR (in GMSO).

PR Checklist


daico007 commented 2 years ago

still pending unit tests

bc118 commented 2 years ago

The 1-4 values seem to be printing correctly now. I think the following command must print all the 1-2, 1-3, and 1-4 values: top.scaling_factors = self.ff.scaling_factors (for me only printing 1-4).

or

the command fails in the foyer topology file at " if item not in scaling_factors.keys():" location as all the keys are not provided. Note that I have been manually entering all the recent changes so it is possible I made an error.

at ~ line 250: @scaling_factors.setter def scaling_factors(self, scaling_factors): """Set the scaling factors for the topology.""" scaling_factors = {'electrostatics12Scale': 0.0, 'nonBonded12Scale': 0.0, 'electrostatics13Scale': 0.0, 'nonBonded13Scale': 0.0, 'electrostatics14Scale': 0.0, 'nonBonded14Scale': 0.0} expected_items = [ "nonBonded12Scale", "nonBonded13Scale", "nonBonded14Scale", "electrostatics12Scale", "electrostatics13Scale", "electrostatics14Scale", ] if not isinstance(scaling_factors, dict): raise GMSOError("Scaling factors should be a dictionary") for item in expected_items: if item not in scaling_factors.keys(): raise GMSOError( f"Expected {expected_items} as keys in the scaling factors" )

bc118 commented 2 years ago

Not sure if this is foyer or gmso, but putting it here. When you add the 1-2 and 1-3 scaling factors to this area in the GMSO FF XML, you get an error. I would imagine that this is where this should go anyway. Not critical as mostly always 0, but wanted to note:

area in the gmso FF XML: FFMetaData electrostatics14Scale=“0.0” nonBonded14Scale=“0.0" >

CalCraven commented 2 years ago

LGTM! The only think I might consider are some more unit tests. These are similar to ones already used for the foyer.forcefield.Forcefield apply method, and use some similar files. See the attached zip.

Also, the lj13scale in the benzene_combine.xml does not get read over, see the last commented out tests in ConvertFoyerGMSOmetdata.ipynb. Not sure if that's intentional at this stage, or if it should be addressed in this PR as well. foyer489.zip