Skip to content
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

Improve robustness of restraint search and fix torsional force constant bug #360

Merged
merged 6 commits into from
Oct 29, 2024

Conversation

fjclark
Copy link
Contributor

@fjclark fjclark commented Oct 28, 2024

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

This PR does two things:

  1. Fixes ABFE restraints torsional force constants are too high #359
  2. Improves the robustness of the ABFE restraint search to strange user input. I've implemented the improvements discussed here.

I've included the two together to reduce CI runs/ faff, but please let me know if I should create separate PRs in future.

If this is to fix a bug...

This pull request fixes issue #359?

  • 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 permission to release this code under the GPL3 license: [y]

Note that the ordering of candidate anchor points in the restraint search will be slightly different now, as the torsions have less weight when ordering the restraints for selection.

If this introduces new functionality...

Changes proposed in this pull request:

I now:

  • Ensure that anchors can never be in the same molecule
  • Warn the user if they select the protein/water as the "ligand"
  • Warn the user if they supply very few frames for fitting
  • 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 confirm that I have permission to release this code under the GPL3 license: [y]

The tests could be more comprehensive - at the moment I've only added a test to check that an error is raised if the receptor and ligand candidate anchor point lists share atoms. I haven't included tests to check that the correct warnings are being raised. The reason for this was that the only trajectory I can find on the website (https://github.com/OpenBioSim/biosimspace_website/tree/main/structures) only includes the ligand and the protein. This means that I can't select weird "ligands" which aren't the ligand or protein (e.g. water) which don't raise errors for other reasons. If we'd like more complete tests, we could always add a trajectory with e.g. 1 water molecule as well as the complex. However, I have verified that these warnings are raised for one of my test systems, and the code for these checks is very simple.

Suggested reviewers:

@lohedges

Thanks.

lohedges and others added 4 commits October 24, 2024 17:14
Previously, the variance of torsions was being calculated as ~3 times
greater than it should have been. Now, the selected torsional force
constants will be ~3 times lower, and slightly different anchor points
may be selected as the torsions will have slightly less weight when
ordering candidate anchor points. However, this does not affect the
correctness of any previous ABFE calculations, and stability should not
be affected (as this is related to the angle restraints, and not the
torsional restraints).
Three checks have been added:

1. An error is raised if the receptor and ligand candidate anchor
   points share no common atoms.
2. The user is warned if few frames (< 50) are supplied.
3. The user is warned if the "ligand" is water or a macromolecule.
@lohedges
Copy link
Contributor

Thanks for this. The CI is failing because there isn't yet a 2024.4.0.dev Sire package. I'll trigger the CI for that now.

@lohedges
Copy link
Contributor

Not sure what's going on, but it appears that you might have merged in devel without actually taking the changes, i.e. using ours. This is why the CI is failing, since you are still building against the old version of Sire. (Although the packages should still be available.) For example, this requirements files appears to be the one from main, not devel. I think this might also explain the weird diff with the website index file, which is proposing changes which are already in devel.

@fjclark
Copy link
Contributor Author

fjclark commented Oct 28, 2024

Sorry, not sure what I've done to cause this. Double-checking that my devel was in sync with OBS, pulling again, and merging again changed only requirements.txt:

image

@lohedges
Copy link
Contributor

No problem. Things look correct now. Ignore the Windows failure, although the CI passed, it crashed when uploading the package to the Anaconda cloud. The Linux 3.12 failure is due to black formatting. (I need to switch to pre-commit at some point so the linting/formatting is automated.)

Copy link
Contributor

@lohedges lohedges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @fjclark. This looks great and will make the restraint selection a lot more reliable.

@lohedges lohedges added the bug Something isn't working label Oct 29, 2024
@lohedges lohedges merged commit 4852202 into OpenBioSim:devel Oct 29, 2024
5 checks passed
lohedges added a commit that referenced this pull request Oct 29, 2024
lohedges added a commit that referenced this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ABFE restraints torsional force constants are too high
2 participants