-
Notifications
You must be signed in to change notification settings - Fork 15
Fix issues #366 and #367 #368
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
|
Looks like this has caused a failure with a distance restraint unit test. I've also realised that the AABox approach won't always work for ring breaks transformations, since it could massively underestimate the extent of the molecule, hence the size of the box that is needed. |
akalpokas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this on my end and I can see the dynamic box size resizing in the vacuum leg, with the box vectors increasing from 20 A to 44 A. Thanks for the changes!
|
Great, thanks for testing. This should cover most basic perturbations but we'll still probably need to be careful for macrocyclisation transformations. In that case, setting an appropriate box should be part of the setup process. I did think about changing the code to raise an exception when no box was set, rather than setting a better default. However, quite a few tests and bits of the tutorial use no box, so it would require a lot of changes. |
This PR closes #366 by resetting the GCMC water state prior to minimisation following a dynamics crash. Previously the state was set after minimisation, meaning that minimisation would be performed on an OpenMM system where all waters were real, so large steric clashes could occur.
The PR also closes #367 by setting sensible defaults for the OpenMM periodic box vectors when the Sire system doesn't have a periodic space. This uses the AABox of the system coordinates to work out a minimum box size, then adds twice the nonbonded cutoff distance.
The PR has also removed some redundant code from the restraints, which was probably copied across from one of the others where it is used.
develinto this branch before issuing this pull request (e.g. by runninggit pull origin devel): [y]@akalpokas: Could you take a look at this next week? We can update the periodicity of an restraints in this PR too once tested.
Cheers.