-
Notifications
You must be signed in to change notification settings - Fork 18
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
Virtual sites missing from gradient calculations #486
Comments
Could you write up a minimal reproduction of this? My guess is that Interchange is failing to create some of the virtual sites, but it's also possible that something is misfiring with the gradient keys. |
Just bumped into this again when trying to fit to mixture properties including water. Here is a small example "Make a sub system with evaluator for a mixture with a water 4 point vsite model"
from openff.evaluator.utils.openmm import system_subset
from openff.toolkit import Molecule, Topology, ForceField
from openff.evaluator.forcefield import ParameterGradientKey
import openmm
# make a topology for a mixture of ethane and water
top = Topology.from_molecules([Molecule.from_smiles("O")])
ff = ForceField("tip4p_fb-1.0.0.offxml")
p_key = ParameterGradientKey(tag="VirtualSites", smirks="[#1:2]-[#8X2H2+0:1]-[#1:3]", attribute="distance")
system, p_valu = system_subset(parameter_key=p_key, force_field=ff, topology=top, scale_amount=0.2)
with open("system.xml", "w") as output:
output.write(openmm.XmlSerializer.serialize(system))
so I think that all electrostatic handlers and subclasses of them should be included when we try to fit any electrostatic parameter. |
If I understand your suggestion correctly, is the idea is add this logic to |
I think the logic should be in I think the main difficulty is finding all the related parameters to the target. For example, fitting the v-site properties in mixtures will require the AM1BCC charge handler if that was used to parameterise the other molecules and when plugin handlers are involved it becomes more complicated again, how would we ensure we have included all present plugins which are related? |
I think I misunderstood the problem originally; the error bubbles up from Interchange's rule about needing an Does the gradient-fitting routine work when a "subset" still contains so many handlers? Here's where my lack of actual knowledge of the code and science limits me. |
Evaluator would need to provide all present handlers which control the same interaction as the one we want the gradient of including plugins. I can see two possible ways we could do this:
I think so if we go with case two even though we have to compute all the electrostatics we still get to skip all bond angle and torsion terms which should help. |
I think there's a number of things that we could do here, and I'm not sure which to pick. Here's what I'm able to come up with now:
These are ordered in increasing utility and, I figure, also in increasing difficulty. I've started #534 which at least gets your snippet above running without error. I don't grasp if that's enough to get gradient-based fitting actually working, but it's a start. |
@jthorton have you been able to use this (it should be in the most recent release)? Any feedback on performance or breaking cases? |
I think that virtual sites are currently missing from property gradient calculations but should be included if present. Currently, evaluator tries to make a system with a reduced number of force field parameters when calculating the gradients of properties here and tries to make sure that any interdependencies between parameters are accounted for but virtual sites seem to be missed. I think they should be included and it would be great if not just that handler is included but also any subclasses of it are as well, this is related to the custom vsite handler we currently use in QUBEKit and this fix in the toolkit might be relavent here.
The text was updated successfully, but these errors were encountered: