Develop ITP reader and writer methods (and corresponding tests) for other forcefields#52
Merged
lunamorrow merged 14 commits intomainfrom Feb 7, 2025
Merged
Develop ITP reader and writer methods (and corresponding tests) for other forcefields#52lunamorrow merged 14 commits intomainfrom
lunamorrow merged 14 commits intomainfrom
Conversation
lunamorrow
requested review from
RangikaM,
khiron and
recombinatrix
as code owners
…this enables the same function to handle all forcefield formats and direct resolving format differences (i.e. in the way dihedrals may be specified) to lower level class methods
…leculetype ]' in the output ITP if it has a value in the Topology - writing of 'None' if there was no attribute was causing issues when re-reading in an ITP
…e tested - mainly via expansion of Dihedral.__str__ representation to display constants when written to ITP file if the dihedral type specifies it
… and base tested - via expansion of Dihedral, Bond and Angle classes from_line() methods and __str__ representations. Also added an additional test to check that the from_ITP method rejects invalid 'format' argument strings
…d a shuffling method and appropriate warning messages to Dihedral initialiser, where the order of atom B and atom C is shuffled and retried if the angles inside the dihedral cannot be resolved. Other changes to Dihedral __str__ representation for the periodic improper dihedral type
Extending support for other forcefield itp files uncovered a bug where undefined, random failure to resolve a dihedral containing the new bond was observed during polymer extension. The cause of this bug was due to the Bond.from_atoms static method, which would find all bonds between a pair of atoms, and if there was more than 1 (in the case of a duplicated bond between two joining atoms during extension) it would return the second bond. This method is envoked to find bonds which will be assigned angles (and thus dihedrals) when a leaving 'residue' atom is replaced by a new 'monomer' atom. As a result, duplicate bonds were found to share half of the angles each, which caused issues when attempting to resolve dihedrals. This was corrected by ensuring the method returns the bond with angles associated if there are duplicates. However, after polymer extension is complete, the bonds are 'deduplicated' to remove a duplicate, and the duplicate that is removed would be the second one iterated through in Atom 'deduplicate_bonds()' -> thus if the second of the two bonds was the one with the angles it would be lost and also result in issues resolving dihedrals in the next polymer extension step. I believe this bug only surfaced now as the test case I was using had a two-atom repeating backbone, so the angles of the bond created in the previous extension step were needed to resolve the dihedrals in the current extension step. I suspect it may also be attributed to changes in dihedral ordering from introducing support to other forcefields, but I am not sure.
Changes made: deduplicate_bonds() uses Bond.from_atoms() to select the correct, 'empty' bond to delete
Bond.from_atoms() has an optional argument used to specify if it should return the empty bond or the bond with angles, returns the bond with angles by default
…changes apparent in the output after large bug fix
… originating topology monomers
…e originating topology monomers
…y should only extend a Polymer with a Monomer of the same format (e.g. gromos-gromos, NOT gromos-amber)
…le originating topology monomers. Modified the __str__ representation choice in Angle to ensure output itp is correct and writes without error, and how the 'missing' bond attributes in CHARMM ff itp's are stored in a Bond to enable extend() to work.
9 tasks
Contributor
Author
|
Merging changes into main and closing this PR. AMBER, CHARMM and OPLS force fields are now supported by PolyTop. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
In reference to #49 and an extension of #48 as this is a large change and needs its own PR. This PR will introduce functional support of the AMBER, CHARMM and OPLS forcefields to PolyTop, on top of the existing support for GROMOS.
Development checklist:
Extended checklist: