Add Bond Order information to Topology

[Lnaden]

This PR ports the OpenMM bond order representation into MDTraj. Implements the Bond class to Topology and updates the Mol2 reader to use bond_order field.

This does change the API a bit since bonds are now represented by a class instead of just two atoms. However, most backwards compatibility should be preserved as each Bond is subclass of the namedtuple so you can iterate by for a,b in top.bonds and still get a Atom class for a and b. This behavior is only different in the to_dataframe output (see below).

There are a couple of points which I implemented which I would like some additional feedback on.

1. Equality/Less than/Greater than of a Bond: Right now I only compare Atom index and not Type/Order since defining these does not change which atoms are bound. I could compare these, but it seemed redundant unless you wanted to define two bonds for the same pair of atoms? The only place I see this being used is in Topology equality checks.
2. to_/from_dataframe representation of Bond. I changed the output of this to float to represent the Bond.type as fractional for things like Aromatic and Amide, without having to have a numpy array of mixed type or structured arrays. Since this is the main API breaking component, it would be good to get some feedback on this implementation and if others have a better way of making this conversion.

Fixes #1307

[Lnaden]

cc @andrrizzi @jchodera as this is a feature related to our projects.

[Lnaden]

The Travis failures appear to be OSX image problems with Ruby version on Brew, I think the fix is just brew update at the start, but that should be its own PR, or wait for travis to update their build image. The Appveyor builds are not running but I don't know why, it appears PyYaml dependencies are failing in an infinite loop, but its unclear why they would have started failing now and not in older PR's.

[rmcgibbo]

could you add a test to ensure that the topology is unchanged by a pickle cycle?

[Lnaden]

I modified the test_pickle of test_topology.py to test equality of a topology with and without bond order. However, there may be gaps in this test because its only checking the atom indices for Bond.__eq__, not order or type as combination of two topologies with bonds between the atoms, but differing order and type properties is ambiguous (see Point 1 of the original post).

With respect to the original post, any feed back on the two outstanding points I asked about?

[rmcgibbo]

For 1. it seems to me that either it should be illegal to define two bonds between the same pair of atoms, or the comparison operators ought to check the type and order. For 2, I think the (admittedly) slightly-breaking way you did the dataframe is the best way.

[rmcgibbo]

I realize this is a bit pedantic, but I don't think you have a total ordering now since you changed eq w/o changing gt and lt. two bonds that share the same atoms but have different type or order are neither greater than, less than, nor equal to one another under the current code.)

[Lnaden]

Under that condition (same atom indices, but different type/order), that should probably throw an error somewhere since that means there are two Bonds between the same pair of atoms (as you mentioned before). I can't think of a use case where that may be helpful, unless you are defining a type of additional restraint through the "bond" framework of whatever simulation package you are using.

I'm not sure where there would be a good time to check for multiple bonds on the same atoms. The add_bond method could check to see if there is already a bond between atom1 and atom2 in the Topology._bonds list, but will that add too much overhead? The only way you could have 2 bonds on the same pair of atoms is if you manually call add_bond right now; I don't think any of the programatic ways (e.g. from_openmm, join) can cause this case.

If you don't mind the overhead of doing a for a,b in self._bonds each time add_bond is called, I can add this in and then not have to worry about the inequality. Alternately, I can add an absolute order to order and type properties for Bond.__lt__ and Bond.__gt__.

[rmcgibbo]

As I said before, I'm really fine with either it being illegal to have multiple bonds between the same pair of atoms or comparison operators that properly handle multiple bonds between the same pair of atoms. I wouldn't worry about the overhead of checking -- it's easy to optimize if slow.

[rmcgibbo]

you ought not to have to write all of the comparators either -- @functools.total_ordering can fill in most of them

[Lnaden]

I've added in the full inequality check. It should catch all the corner cases. Although come to think of it, I could probably do it simpler by just making a tuple of (atom1.index, atom2.index, type, order) casting type and order to floats, then just doing inequality on the tuple... I'll fix that.

The test which keeps failing looks like something to do with one of the notebooks accessing a remote file, I don't think thats something I broke.

[rmcgibbo]

Yeah, that test is something I was trying to fix in #1315, so don't worry about that.

[rmcgibbo]

+1 for the tuple idea too. much easier.

[rmcgibbo]

I think there might be more tests failing, e.g. starting on line 2928 of https://travis-ci.org/mdtraj/mdtraj/jobs/309785921#L2928.

[Lnaden]

So it did, that is something I broken then. I'll figure that out and make a new push once my local tests pass

[Lnaden]

A couple things:

• namedtuple already has __lt__, __gt__, etc., so @total_ordering did nothing, so I implemented all the logical tests one could do, which includes !=, >=, and <=.
• I realized that if I want people to be able to call add_bond and pass a Singleton to Type, I had to make those available to the __init__ of the package.

[Lnaden]

Grain of salt on the tests passing, only the AppVeyor tests ran, travis never hooked into the commit (f8624c4)

[rmcgibbo]

The topology tests run on windows, so I'm sure it's fine.