-
Notifications
You must be signed in to change notification settings - Fork 663
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
added TopologyObject API #2382
added TopologyObject API #2382
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2382 +/- ##
===========================================
- Coverage 90.16% 90.12% -0.05%
===========================================
Files 177 177
Lines 22553 22642 +89
Branches 2917 2933 +16
===========================================
+ Hits 20335 20405 +70
- Misses 1614 1631 +17
- Partials 604 606 +2
Continue to review full report at Codecov.
|
Hopefully fixes #2386. Added a _SymmetricConnection class for Bonds, Angles and Dihedrals with type checking and values where the first atom index is always smaller than the last. Added type checking to Impropers and a test to ensure that u.impropers returns impropers with the same indices as u._topology.impropers. |
Do you mean similar to how we add Attributes in a flexible manner? Seems conceptually simple to add something like "force_constant", "equilibrium_value", "ryckaert_bellemans_coefficients", ... As long as these are "tags" and we don't really have to make sure that they are consistent between everything, it might be a convenient and extensible way to annotate with force field information. If we want to be consistent then this gets harder... |
GROMACS also can have multiple definitions of the same bonds etc. that are additive, which complicates things. :( |
This last commit addresses #2392, but I don't know why it was written like that in the first place, so possibly not the best idea. There are more test offerings to please the codecov overlord. |
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 would factor out a small PR for #2392 if possible – I could review that one.
It's harder for me to substantially review the bigger part of adding the API. In principle the API looks good to me but I'd want a few more eyes on it from people who have been working on that part of the code. Feel free to ping people – ask for reviews and remind them occasionally.
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 like how you deal with fragment invalidation by making the new add_...
methods members of Universe
. This PR is a major step forward with respect to supporting dynamic topologies.
I only have some minor requests regarding method names and docs.
I didn't have time to review the tests yet, so that's still on the todo list.
13f9e61
to
29736b5
Compare
Thanks for the review, @zemanj! |
29736b5
to
8b29131
Compare
8d0d130
to
3600ed3
Compare
4f94b53
to
76adbb1
Compare
97da1ed
to
206652b
Compare
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.
Ok I did a little tidying and added back the improper dihedral stuff, but I think this is good to go now
Thanks @richardjgowers! |
self = <MDAnalysisTests.topology.test_top.TestPRMParser object at 0x7f7087f38860>
top = <MDAnalysis.core.topology.Topology object at 0x7f7087f4a390>
def test_improper_atoms_bonded(self, top):
vals = top.bonds.values
for imp in top.impropers.values:
for b in ((imp[0], imp[2]), (imp[1], imp[2]), (imp[2], imp[3])):
> assert (b in vals) or (b[::-1] in vals)
E assert ((31, 43) in [(10, 11), (10, 12), (4, 6), (4, 10), (0, 4), (25, 26), ...] or (43, 31) in [(10, 11), (10, 12), (4, 6), (4, 10), (0, 4), (25, 26), ...])
testsuite/MDAnalysisTests/topology/test_top.py:127: AssertionError Ah, this is the test that kicked off the symmetric improper dihedral ordering issue in #2386. |
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.
Looks good to me, @zemanj are you happy with this?
That doesn't seem too inconvenient. The code now ensures you cannot add or delete a TopologyGroup, list of TopologyObjects, or list of AtomGroups, from a different Universe. |
It'd be nice to have an easy way to add singular bonds/angles/dihedrals/impropers.
Changes made in this Pull Request:
To universe.Universe:
To topologyattrs._Connections:
PR Checklist
types
keyword for in_Connection.add_bonds
, and topology objects in general? Isorder
a reference to bond order -- what meaning does this have for angles?By the way, I suppose you wouldn't be interested in 'tagging' TopologyObjects with extra information like force constants, etc. from topology files with that information?