-
Notifications
You must be signed in to change notification settings - Fork 15
Add inverse bond/distance restraint and fix issue #379 #380
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
Conversation
|
Not sure what happened, but it appears no test input files could be read. Everything worked locally before pushing. Guess GiHub might have had a connection issue. |
|
The PR also adds support for specifying the OpenCL platform index when creating an OpenMM context via the @mark-mackey-cresset: Tagging you for reference. Let me know if there are any other platform related options that need exposing, other than those that are already available. (See here for a list.) |
|
The |
5c3d45e to
3189d66
Compare
|
I've now done this the hard way. In addition, I realised that the automatic coalchemical restraints generated by SOMD2 need to be passed through in the map as a separate property, i.e. |
|
This is now working and I've tested the
for a0, a1 in zip(atoms0, atoms1):
if use_pbc and a0.molecule() == a1.molecule():
warnings.warn(
"You are adding a bond restraint between two atoms in the same molecule. "
"Using periodic boundary conditions is not recommended."
) |
No, that should be fine. We've never had an issue with OpenCL context options, it's just there were some comments in our code about somd1 behaviour and I wanted to check that they were still valid for somd2. |
|
I'll merge so I can get on with the release. We can evaluate defaults over the next release cycle. Everything can be update via the Python API, or updated defaults in the class headers, so no rebuilding of wrappers etc. |
This PR adds support for
InverseDistanceRestraint(s)andInverseBondRestraint(s), which allows two atoms to be kept apart. This is useful for charge change perturbations where it is desirable to keep alchemical ions away from the perturbing ligand.The PR also closes #379 by allowing the user to specify whether periodic boundary conditions should be used when creating restraints via the new Sire Python API. By default, positional and distance restraints use PBC, and bonded restraints don't. This is needed to avoid issues with bonded restraints being incorrectly satisfied across the periodic boundary, e.g. when a broken bound is being restrained.
I decided to only fix this via the new Python API for simplicity, i.e. by adding a
_use_pbcattribute to the restraints objects returned by the functions insire.restraints, then passing this through to the Sire-to-OpenMM conversion layer via the map. This avoided the need to update all of the corresponding restraint classes inSire::MM, along with adding new versions for all streaming operators. This could still be done in future, since the Python API wouldn't need to change.I've added a unit test that confirms that the PBC flag is set correctly for different restraints and that this propagates through to the OpenMM force. In particular, I've tested that the
BondRestraintForce, which can be used for distance and bond restraints, is set an appropriate default depending on whether it is used as a distance or bond restraint, i.e. distance does use periodicity, bond doesn't. If we strictly want these to be synonyms, then I can standardise the behaviour. My interpretation was that bond refers to physically bonded atoms, whereas distance can refer to any two atoms, although I guess they could still be part of the same molecule, so issues could occur with PBC. (Maybe we could check for this if needed, then set a default based on whether they are in the same molecule?)This will be the last PR before the 2025.3.0 release. Following this, I'll create a new BioSimSpace release, then initial conda packages for our other tools, i.e.
ghostly,loch, andsomd2. This will allow everyone to work with a consistent set of packages for the purposes of running initial benchmarks.develinto this branch before issuing this pull request (e.g. by runninggit pull origin devel): [y]@akalpokas: Tagging you for reference.