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

added TopologyObject API #2382

Merged
merged 26 commits into from
Jan 21, 2020
Merged

Conversation

lilyminium
Copy link
Member

@lilyminium lilyminium commented Oct 29, 2019

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:

  • added add_TopologyObject/s
  • add_Bond/s is a convenience method
  • added delete_TopologyObject/s (have not added delete_Bond/s)
  • added refresh_fragments

To topologyattrs._Connections:

  • added delete_bonds
  • moved type checking to decorator

PR Checklist

  • Tests?
  • Docs?
    • What is the types keyword for in _Connection.add_bonds, and topology objects in general? Is order a reference to bond order -- what meaning does this have for angles?
  • CHANGELOG updated?
  • Issue raised/referenced?

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?

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #2382 into develop will decrease coverage by 0.04%.
The diff coverage is 95.95%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ
package/MDAnalysis/core/topology.py 100% <ø> (ø) ⬆️
package/MDAnalysis/core/groups.py 98.13% <100%> (-0.1%) ⬇️
package/MDAnalysis/core/topologyobjects.py 98.13% <100%> (-0.37%) ⬇️
package/MDAnalysis/core/topologyattrs.py 98.11% <94.11%> (+0.49%) ⬆️
package/MDAnalysis/core/universe.py 95.44% <96.72%> (+0.2%) ⬆️
package/MDAnalysis/analysis/base.py 93.57% <0%> (-3.67%) ⬇️
package/MDAnalysis/coordinates/TRZ.py 82.15% <0%> (-1.49%) ⬇️
package/MDAnalysis/analysis/lineardensity.py 62.38% <0%> (-0.92%) ⬇️
package/MDAnalysis/lib/util.py 87.68% <0%> (-0.88%) ⬇️
package/MDAnalysis/analysis/gnm.py 95.38% <0%> (-0.77%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72ce35a...a14dd93. Read the comment docs.

@lilyminium lilyminium changed the title added add_TopologyObjects added TopologyObject API Oct 30, 2019
@lilyminium
Copy link
Member Author

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.

@orbeckst
Copy link
Member

orbeckst commented Nov 2, 2019

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?

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...

@lilyminium
Copy link
Member Author

GROMACS also can have multiple definitions of the same bonds etc. that are additive, which complicates things. :(

@lilyminium
Copy link
Member Author

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.

Copy link
Member

@orbeckst orbeckst left a 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.

package/MDAnalysis/core/topologyobjects.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved
package/CHANGELOG Outdated Show resolved Hide resolved
Copy link
Member

@zemanj zemanj left a 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.

package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/topologyattrs.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/topologyattrs.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved
package/MDAnalysis/core/universe.py Show resolved Hide resolved
@lilyminium
Copy link
Member Author

Thanks for the review, @zemanj!

Copy link
Member

@richardjgowers richardjgowers left a 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

@lilyminium
Copy link
Member Author

Thanks @richardjgowers!

@lilyminium
Copy link
Member Author

lilyminium commented Jan 14, 2020

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. It might be better to remove this test; if the central atom can be either first or third, you can no longer assume bonding anyway. Extended it to check both forwards and backwards.

Copy link
Member

@richardjgowers richardjgowers left a 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?

@lilyminium
Copy link
Member Author

For the identical topology case I'd sooner make them use u2.add_angles(u1.angles.to_indices())

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.

@richardjgowers richardjgowers dismissed zemanj’s stale review January 21, 2020 11:11

Comments addressed

@richardjgowers richardjgowers merged commit 972a8b1 into MDAnalysis:develop Jan 21, 2020
@lilyminium lilyminium deleted the add-bonds branch April 14, 2020 08:13
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.

4 participants