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

Feature minimise #162

Closed
wants to merge 9 commits into from
Closed

Feature minimise #162

wants to merge 9 commits into from

Conversation

chryswoods
Copy link
Contributor

Optimised the OpenMM minimisation code and making it more robust.
This includes vectorising for Apple Silicon and adding more tests for
convergence so that we can have more confidence that the structures
output are sensible. Also made sure that optimised compilation (-O3) is
used for all of the plugins (SireOpenMM, SireGemmi and SireRDKit).
They were previously compiled with wrapper options (e.g. -Os).
Minimisation now gives better progress updates, using a progress
bar to show progress towards the maximum number of iterations.
This has been reduced to 1500 by default. Also, if the minimisation
fails to create a structure that obeys constraints on the first pass,
then the minimisation is repeated, with the maximum number of
iterations reset. If it fails again, then this structure, with
constraints re-applied, is returned.

  • 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/a (existing tests validate this)]
  • 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/a (existing documentation describes this)]
  • 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): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@lohedges, @mb2055

Any additional context of information?

It would be worth checking that minimisation is stable for a number of systems before accepting this PR. This makes the QM/MM alanine dipeptide system stable ("ala_25_0.rst7", "ala.top") when minimised. But, I have noticed that some TYK2 systems can be difficult to minimise (the algorithm has to try several times, and you can see progress pausing as the line search evaluates a lot without moving forwards)

This includes the LambdaSchedule bug fix from the other PR.

…ixed a bug

in the new functionality where a default lever for a force was not being picked
up (i.e. `ghost/ghost::*` was not taking precedence over, e.g. `*::charge`)

I've updated the unit tests to catch these edge cases
…times to find a stable

starting dynamics structure. This solves the weird aladip structure problem, but
runs minimisation too long for tyk2 (getting confused when constraints are re-applied)
…ore robust

with the way constraints are handled.
This includes vectorising for Apple Silicon and adding more tests for
convergence so that we can have more confidence that the structures
output are sensible. Also made sure that optimised compilation (-O3) is
used for all of the plugins (SireOpenMM, SireGemmi and SireRDKit).
They were previously compiled with wrapper options (e.g. -Os).
Minimisation now gives better progress updates, using a progress
bar to show progress towards the maximum number of iterations.
This has been reduced to 1500 by default. Also, if the minimisation
fails to create a structure that obeys constraints on the first pass,
then the minimisation is repeated, with the maximum number of
iterations reset. If it fails again, then this structure, with
constraints re-applied, is returned.
@lohedges
Copy link
Contributor

Thanks, this sounds great. Looking at the actions there appear to be some build issues. I'll test from the QM/MM side once I've merged devel across to my branch. I just need to resolve conflicts and switch to the new auto-generation of wrappers. I'll let you know if I have any questions.

Ran the wrapper generation again, which added docs to PerturbableOpenMMMolecule

Moved the optimised cxxflags to a higher-level CMakeLists.txt and added this properly,
rather than duplicating flags. Will see if this fixes the Linux compile error.

(will check locally, hence skipping CI)

[ci skip]
…ebugging info

to minimisation output for problematic cases
minimisation to the user via arguments with sensible defaults to the
minimisation functions. The documentation of the functions explains
how the ratchetting algorithm works. Also updated the progress
bar so that you can see the number of repeats and number of
ratchets (e.g. 1.5 would be the sixth ratchet of the first run)

Also fixed the double-header output for the create_wrapper script
and regenerated the SireOpenMM wrappers. This also now automatically
finds the python header files.
@lohedges
Copy link
Contributor

I've been testing on feature_emle and it's working superbly. I imagine that we can try different parameters for some of the problem systems, e.g. tyk2, to see if we can come up with a best case protocol. (Or have something that is adaptive.)

@chryswoods
Copy link
Contributor Author

That's brilliant - I've played some more and now have a good handle on the error codes that the underlying algorithm spits out. These tell you when you need to change algorithm parameters (e.g. increase line search steps etc). I am adding in some logic to handle these automatically, which is helping deal with the troublesome tyk2 cases. I'm going to add a bit more robustness to this PR before I merge it. My goal is to get the minimisation to work for everything except truly pathological cases ;-)

@chryswoods
Copy link
Contributor Author

I'll close this now so I don't accidentally trigger CI, and will reopen when I've finished.

@chryswoods chryswoods closed this Feb 14, 2024
@chryswoods chryswoods mentioned this pull request Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants