More Flexible Creation of Boresch Restraints

[fjclark]

This PR increases the flexibility of Boresch restraint creation. I've created this as a first draft so we can discuss how best to do this. I've not yet added tests as I'm currently unable to compile sire and test properly (will create an issue).

Changes proposed in this pull request:

• Allow the user to specify the equilibrium values for the Boresch restraint, in addition to the force constants. All equilibrium values specified as None are measured from the input structure.
• Add further checks on the likely stability of the Boresch restraints. This is mostly copied from the BSS code here.
• Add a long docstring explaining all arguments and including an example.
• Change the default values of the force constants to 10 kcal mol-1 A-2 for the distance and 100 kcal mol-1 rad-2 for bond angles/ dihedrals. These values are closer to those we see when fitting force constants to simulation e.g.:
  
  These values are also more in-keeping with defaults in the literature, e.g. the FEP+ implementation where they use much higher angle force constants (40 kcal mol-1 rad-2) compared to distance (1 kcal mol-1 A-2).

Some points to discuss:

• Is the way I've written this (trying to do things in a loop for each restraints component) worthwhile, or have I just made things more complex and harder to read?
• How should we implement the standard state corrections? Two possible options are a) expose more information from the underlying C++ class so that we can feed the restraint produced by the boresch function to a get_standard_state_correction function b) create classes in the python layer which store all the force constants and equilibrium values, and have a standard_state_correction method/ property.
• Is the way that the user supplies restraint information (lists of force constants and equilibrium values) best? I like that the user can supply a single force constant which is copied over to all remaining angle/ dihedral restraints, but maybe using a dictionary similar to the current SOMD1/ BSS implementation would be clearer?

• I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): y
• I confirm that I have added a test for any new functionality in this pull request: n - will do this once able to compile sire
• I confirm that I have added documentation (e.g. a new tutorial page or detailed guide) for any new functionality in this pull request: n
• I confirm that I have added a changelog entry to the changelog (we will add a link to this PR as part of the review): n
• I confirm that I have permission to release this code under the GPL3 license: y

Suggested reviewers:

@chryswoods, @lohedges

Thanks very much!

[fjclark]

I've now made all the changes I'd like:

• Tests added for creating Boresch restraints
• Analytical standard state correction for Boresch restraint
• Added tutorial section on Boresch restraints
• Added reading of arguments from the passed map (unsure if this is exactly what you were thinking, so please let me know if I've misunderstood)

I had a couple of questions:

• Using map = create_map(map) overwrites the python built-in map function. Should we call it something else to avoid this?
• I was wondering why boresch returns BoreschRestraints rather than a single BoreschRestraint? I can see that this is more consistent with e.g. positional where we often want to pass many restraints, but it seems unlikely that the user would want more than one set of Boresch restraints in most cases.

Thanks!

[jmichel80]

“but it seems unlikely that the user would want more than one set of Boresch restraints in most cases.”

Some dual topology implementations could use two sets of Boresch restraints on two separate ligands (e.g. SepTop method). But this could also be implemented by setting restraints on one ligand at a time.

[chryswoods]

Thanks - the clash with map is unfortunate, but it is baked into sire from decades back, and can't be changed. Fortunately, the overwrite occurs only in the functions that take "map" arguments, which themselves aren't going to call the Python map function.

[lohedges]

Using map = create_map(map) overwrites the python built-in map function. Should we call it something else to avoid this?

This is why I ended up using property_map throughout in BioSimSpace so that I didn't shadow built in functionality. (That said,t he map is now far more general so property_map isn't the sole use case anymore.) However, I always accidentally type map since it's so baked into my brain from the Sire side of things :-) As @chryswoods, these are always passed as an argument to a function, or used internally, so the likelihood of a clash is small. (Although the naming might confused some Pythonistas.)

[chryswoods]

For some reason this failed because it timed out (all runners hung for 6 hours). I am not sure why. Is there anything that has been added that may have caused this? If not, I can take a look later to see what could have gone wrong.

[fjclark]

I'm not sure what I could have changed. To double-check, I pulled in the latest changes from devel and tested again locally. Other than a couple of unrelated errors (related to gemmi (E TypeError: Unregistered type : gemmi::Structure) - I assume I need to update gemmi?), everything passes. I've pushed the merged version to see if it works this time.

[chryswoods]

Thanks - this looks great. I really like the docs and tests.

I've pulled this into #fix_164 so that we don't have to remerge in devel again and to save CI. Your PR will be merged in with #fix_164 (which should be soon).

[fjclark]

Fantastic, thanks very much.

[chryswoods]

Thanks - this is now merged :-)