Fix #228 #229
Merged
Fix #228 #229
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Based of Boresch restraints for RF in src/libs/SireMove/openmmfrenergyst.cpp and 43ed4bc
…-pme' into fix_somd_pme_torsions
lohedges
added
bug
|
Do you mind make a branch targeting main so I could have a test? Thanks. |
lohedges
added a commit
that referenced
this pull request
Aug 20, 2024
lohedges
added a commit
that referenced
this pull request
Aug 20, 2024
|
Here we go. |
Updated to match the equation in openmmfrenergyst.cpp. This is due to a minor issue with the older version where the potential is not harmonic with equilibrium position dl.
|
@xiki-tempula: Did you get a chance to test this? If all is okay, then I'll merge into |
|
Yes. It works very well. |
|
Perfect. Thanks for confirming. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR closes #228 by correcting the logic to set GROMACS atom types for dummy atoms. In certain cases an additional
_dusuffix was applied to dummies when writing to the[ atoms ]section, meaning that no match was found in the[ atomtypes ]section. In addition, atom type deduplication wasn't always working correctly meaning that there were (sometimes) unused atom types in the[ atomtypes ]section, i.e. they never appeared in[ atoms ]. This meant that an atom in[ atoms ]may have been paired with the wrong type from[ atomtypes ]. (This doesn't matter for all terms, since those in[ atoms ]take precedence.)I'm still not sure how this issue hasn't occurred before. It's not clear whether this was new input for Exscientia, or whether an existing test triggered the problem following the LJ sigma update. (Although I can't see how.)
I've added a unit test to validate the two sets of records for the problem system and have also checked that all of the BioSimSpace unit tests still pass.
Note that I'd probably like to go back and simplify the logic here at some point. This is a quick fix to get things working before I go on holiday.
develinto this branch before issuing this pull request (e.g. by runninggit pull origin devel): [y]Suggested reviewers:
@chryswoods