Feature Multiple Distance Restraints #178

fjclark · 2023-10-02T16:45:33Z

Is this pull request to fix a bug, or to introduce new functionality?

This PR introduces functionality for setting up and running ABFE calculations with multiple distance restraints. This requires the functionality in this Sire PR.

If this introduces new functionality...

Changes proposed in this pull request:

Extend the Restraint class to hold multiple distance restraints and to write input files for calculations of this type with SOMD
Implement a restraint search algorithm for multiple distance restraints. This currently:
- Selects distance restraints for all heavy atoms in the ligand
- Prioritises restraints between pairs of atoms with low SD of distance based on a simulation of the fully interacting complex
- Promotes diversity of direction of the distance restraints by clustering candidate restraints by direction and selecting only one restraint per directional cluster
- Chooses the flat-bottomed radius of the restraints to encompass 95 % of the distance sampled during the simulation of the fully interacting complex, so that the free energy of introducing the restraints is negligible

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: y
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 will added a section to the ABFE tutorial if approved)
I confirm that I have permission to release this code under the GPL3 license: y

When testing locally, all tests that I've added or that are related to functionality that I've changed pass. However, the following tests fail:

FAILED tests/Sandpit/Exscientia/Trajectory/test_trajectory.py::test_rmsd - AttributeError: 'TrajectoryIterator' object has no attribute 'rmsd'
FAILED tests/Sandpit/Exscientia/_SireWrappers/test_system.py::test_set_box - assert False
FAILED tests/Sandpit/Exscientia/_SireWrappers/test_system.py::test_rotate_box_vectors - AttributeError: 'TriclinicBox' object has no attribute 'rotate'

Suggested reviewers:

@lohedges, @chryswoods

Any additional context of information?

As shown in the Sire PR, so far my tests show no evidence of a significant difference compared to Boresch restraints. My test input scripts are here.

Thanks very much.

This includes specifying the format of the restraints dictionary and implementing the numerical correction.

Allow correct SOMD config and pert files to be written for multiple distance restraints. Accompanying tests have also been introduced. This involved introducing an additional perturbation type "release_restraint", used exclusively with multiple distance restraints to remove all distance restraints but the "permanent" distance restraint.

Ensure that "endmolecule" is written for all SOMD pert files, and make the tests on the SOMD pert files more detailed (added checks for start and end atom and molecule lines).

Increase the number of directional clusters until the restraint search succeeds or hits the maximum.

lohedges · 2023-10-03T12:48:59Z

Thanks, @fjclark:

When testing locally, all tests that I've added or that are related to functionality that I've changed pass. However, the following tests fail:

Since the same tests passed during the CI here, I can only assume that locally you are testing against outdated versions of Sire and BioSimSpace? The TriclinicBox changes are fairly recent, so it's possible that you hadn't pulled them across to your fork.

fjclark · 2023-10-03T14:05:35Z

Yes, thanks - I had pulled the changes but forgotten to reinstall Sire (they now pass as expected).

chryswoods · 2023-10-03T16:16:51Z

Thanks - we are about to release 2023.4.0 (likely end of this week or early next week - it will depend on when we squash these last bugs).

Do you want this included in 2023.4.0? Or do you want more time to test it? I don't mind either way and am happy to review and merge this this week if you are happy for it to be included.

lohedges · 2023-10-03T16:20:39Z

I think @xiki-tempula might like to test this implementation soon, so I guess it might depend on whether he'd prefer that to be in 2023.4.0, or 2024.1.0.dev (or whatever it is).

chryswoods · 2023-10-03T16:27:14Z

2023.4.0 would mean we could further test and then fix bugs as part of 2023.4.1+. Going for 2023.5.0.dev would mean that it will be stuck in the dev package until we release 2023.5.0 (end of December).

The risk of going early is if this regresses or breaks existing functionality in any way. Do we have good regression tests for this code to demonstrate it will be safe to merge?

xiki-tempula · 2023-10-04T06:53:08Z

Thanks for informing me. I'm fine either way but I noticed that the implementation for the new restraint is for SOMD only and our pipeline currently only supports Gromacs, so I might not be testing it in the near future.

fjclark · 2023-10-04T08:12:20Z

Thanks - we are about to release 2023.4.0 (likely end of this week or early next week - it will depend on when we squash these last bugs).

Do you want this included in 2023.4.0? Or do you want more time to test it? I don't mind either way and am happy to review and merge this this week if you are happy for it to be included.

Thanks very much. I think slightly more time to test it would be best. I'll also look into implementing this in Gromacs. I'll let you know once I've done this.

xiki-tempula · 2023-10-04T08:41:19Z

@fjclark Thanks

chryswoods · 2023-10-07T15:48:23Z

I reviewed and merged in the sire part of this so that it would make 2023.4.0 (sorry for not seeing the above first). The changes looked orthogonal to existing functionality, so relatively safe to include. I agree to wait for the next release for the BioSimSpace part - this will give you more time to test the interface before making this feature more public.

This is done by using restraint potentials (function type 10) for the flat-bottomed restraints which are affected by lambda- restraint, and a distance restraint for the permanent restraint which is not affected by any lambdas. See https://manual.gromacs.org/documentation/2019-rc1/reference-manual/topologies/topology-file-formats.html#id31 for details. Note that the distance restraint requires the force constant to be written to the mdp file.

…openbiosim into feature_mdr

…int is supplied

fjclark · 2023-11-18T13:28:46Z

Hi @lohedges, I've now finished implementing this for SOMD and for GROMACS and have validated both implementations by comparing with Boresch restraints. In both cases, I ran 5 independent replicates of the bound leg with each type of restraint and the errors are 95 % t-based confidence intervals:

SOMD

Boresch

Multiple Distance Restraints

However, I observed a couple of crashes during the release restraint stage. To investigate these further, I ran 90 release restraint windows (10 replicates of 9 windows), and observed 10 crashes:

These crashes are sometimes observed when there are no restraints, and seem to occur much more frequently with open force fields compared to GAFF. As no crashes were observed with GROMACS, this seems to be a strange SOMD bug. More discussion is given here.

For now, I've added a warning about potential instabilities whenever multiple distance restraints are used with SOMD.

GROMACS

Boresch

Multiple Distance Restraints

Overall

There is no evidence for a significant difference between Boresch restraints and multiple distance restraints for either engine. @lohedges and @xiki-tempula, once you're happy with this PR I'll add a section to the tutorials showing how to use these. It's slightly different to Boresch restraints for GROMACS as a) The user has to specify restraint-lambda instead of bonded-lambda b) the calculation has to be split into two stages. This is because the only way I could see to do this was using a combination of bond restraint potentials (affected by restraint-lambda) and distance restraints (not affected by restraint-lambda).

This requires the changes to BioSimSpace implemented here: OpenBioSim/biosimspace#178.

jmichel80 · 2023-11-18T17:42:52Z

That’s great work ! There seems to be a surprising difference in precision between the two engines, something to think about.

lohedges · 2023-11-21T09:32:36Z

Thanks @fjclark. Given the tests pass and this only touches sandpit functionality, I'd be happy to merge. Perhaps @xiki-tempula would like to check that it doesn't cause any issues with the internal Exs test suite before merging?

xiki-tempula · 2023-11-21T09:44:22Z

I have skim through the PR and I think it looks ok.

.github/workflows/Sandpit_exs.yml

fjclark and others added 15 commits August 8, 2023 09:38

Update _restraint.py to handle multiple distance restraints

6bae2d5

This includes specifying the format of the restraints dictionary and implementing the numerical correction.

Update names of all Boresch restraint tests

0c78c6b

Check correct keys in multiple distance restraints dict

64dd2aa

Add tests for multiple distance restraints and fix bugs this revealed

fd400b8

Change access of pandas series to avoid deprecation warning

5dc9585

Add first version of multiple distance restraints derivation algorithm

67b4382

Ensure that atom record is always written

8cd3986

Correct SOMD pert files

eb41a90

Ensure that "endmolecule" is written for all SOMD pert files, and make the tests on the SOMD pert files more detailed (added checks for start and end atom and molecule lines).

Merge branch 'devel' into feature_mdr

8d89a05

Fix sandpit CI

0fe2fe4

Fix issue with checking alchemlyb version number

8e722eb

Make restraint search more robust for multiple distance restraints

514370e

Increase the number of directional clusters until the restraint search succeeds or hits the maximum.

Add more tests of mdr restraint search algorithm

f5d1743

Merge branch 'devel' into feature_mdr

16d00d0

fjclark temporarily deployed to biosimspace-build October 2, 2023 16:45 — with GitHub Actions Inactive

lohedges added the enhancement New feature or request label Oct 3, 2023

fjclark added 6 commits November 8, 2023 10:31

Merge branch 'devel' into feature_mdr

a61bbc0

Add tests for MDR restraints dict

9971a7b

Merge branch 'feature_mdr' of https://github.com/fjclark/biosimspace-…

f8098bf

…openbiosim into feature_mdr

Merge branch 'feature_mdr_gromacs' into feature_mdr

a8aeb71

Merge branch 'devel' into feature_mdr

35b237f

fjclark had a problem deploying to biosimspace-build November 14, 2023 13:35 — with GitHub Actions Failure

fjclark temporarily deployed to biosimspace-build November 14, 2023 13:35 — with GitHub Actions Inactive

fjclark had a problem deploying to biosimspace-build November 14, 2023 13:35 — with GitHub Actions Failure

fjclark temporarily deployed to biosimspace-build November 14, 2023 13:35 — with GitHub Actions Inactive

fjclark added 2 commits November 14, 2023 16:57

Require that gromacs process is passed free energy protocol if restra…

e33539f

…int is supplied

Update changelog with MDR implementation

a8b56d0

fjclark temporarily deployed to biosimspace-build November 14, 2023 16:58 — with GitHub Actions Inactive

fjclark temporarily deployed to biosimspace-build November 14, 2023 16:59 — with GitHub Actions Inactive

fjclark temporarily deployed to biosimspace-build November 14, 2023 17:00 — with GitHub Actions Inactive

fjclark temporarily deployed to biosimspace-build November 14, 2023 17:16 — with GitHub Actions Inactive

Add warning about SOMD instabilities with decoupled ligands [ci skip]

6655bfd

fjclark added a commit to fjclark/biosimspace_tutorials_openbiosim that referenced this pull request Nov 18, 2023

Update ABFE to include multiple distance restraints

8a3f4f7

This requires the changes to BioSimSpace implemented here: OpenBioSim/biosimspace#178.

fjclark mentioned this pull request Nov 18, 2023

Update ABFE setup notebook to include multiple distance restraints OpenBioSim/biosimspace_tutorials#14

Merged

lohedges changed the title ~~[WIP] Feature Multiple Distance Restraints~~ Feature Multiple Distance Restraints Nov 21, 2023

xiki-tempula approved these changes Nov 21, 2023

View reviewed changes

xiki-tempula reviewed Nov 21, 2023

View reviewed changes

.github/workflows/Sandpit_exs.yml Outdated Show resolved Hide resolved

Remove duplicate mention of scikit-learn in dependencies [ci skip]

8025d4a

lohedges merged commit 755103e into OpenBioSim:devel Dec 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature Multiple Distance Restraints #178

Feature Multiple Distance Restraints #178

fjclark commented Oct 2, 2023

lohedges commented Oct 3, 2023

fjclark commented Oct 3, 2023 •

edited

Loading

chryswoods commented Oct 3, 2023

lohedges commented Oct 3, 2023

chryswoods commented Oct 3, 2023 •

edited

Loading

xiki-tempula commented Oct 4, 2023

fjclark commented Oct 4, 2023

xiki-tempula commented Oct 4, 2023

chryswoods commented Oct 7, 2023

fjclark commented Nov 18, 2023

jmichel80 commented Nov 18, 2023

lohedges commented Nov 21, 2023

xiki-tempula commented Nov 21, 2023

Feature Multiple Distance Restraints #178

Feature Multiple Distance Restraints #178

Conversation

fjclark commented Oct 2, 2023

Is this pull request to fix a bug, or to introduce new functionality?

If this introduces new functionality...

Suggested reviewers:

Any additional context of information?

lohedges commented Oct 3, 2023

fjclark commented Oct 3, 2023 • edited Loading

chryswoods commented Oct 3, 2023

lohedges commented Oct 3, 2023

chryswoods commented Oct 3, 2023 • edited Loading

xiki-tempula commented Oct 4, 2023

fjclark commented Oct 4, 2023

xiki-tempula commented Oct 4, 2023

chryswoods commented Oct 7, 2023

fjclark commented Nov 18, 2023

SOMD

GROMACS

Overall

jmichel80 commented Nov 18, 2023

lohedges commented Nov 21, 2023

xiki-tempula commented Nov 21, 2023

fjclark commented Oct 3, 2023 •

edited

Loading

chryswoods commented Oct 3, 2023 •

edited

Loading