Openff-toolkit: Make all offxml files compatible

[bannanc]

@ChayaSt attempted to run the example from the README and was running into a weird error, that initially I wasn't able to replicate, but then tried a slightly different version of the same code and we were able to isolate the issue.

We started from a new conda environment with:

conda create -n chaya_test python=3.6
conda activate chaya_test
conda install --yes -c conda-forge -c omnia openforcefield
conda install -c openeye openeye-toolkits
conda install ipython jupyter

Then using the example we made an OEMol from a smiles and loaded the offxml file from the conda installation.

smiles = 'Cc1cccc(c1C)C'
new_mol = oechem.OEMol()
oechem.OESmilesToMol(new_mol, smiles)
oechem.OEAddExplicitHydrogens(new_mol)
FF_filename = get_data_filename('forcefield/Frosst_AlkEthOH_parmAtFrosst.offxml')
ff = forcefield.ForceField(FF_filename)
top, syst, pos  = ff_utils.create_system_from_molecule(ff, new_mol, verbose = False)

This gives this rather confusing error:

Traceback (most recent call last):
  File "test_chaya_error.py", line 15, in <module>
    top, syst, pos  = ff_utils.create_system_from_molecule(ff, new_mol, verbose = False)
  File "/Users/caitlinbannan/miniconda3/envs/chaya_test/lib/python3.6/site-packages/openforcefield/typing/engines/smirnoff/forcefield_utils.py", line 98, in create_system_from_molecule
    system = forcefield.createSystem(topology, [mol], verbose=verbose)
  File "/Users/caitlinbannan/miniconda3/envs/chaya_test/lib/python3.6/site-packages/openforcefield/typing/engines/smirnoff/forcefield.py", line 1022, in createSystem
    force.createForce(system, topology, verbose=verbose, **kwargs)
  File "/Users/caitlinbannan/miniconda3/envs/chaya_test/lib/python3.6/site-packages/openforcefield/typing/engines/smirnoff/forcefield.py", line 1499, in createForce
    _check_for_missing_valence_terms('HarmonicBondForce', topology, bonds.keys(), topology.bonds())
  File "/Users/caitlinbannan/miniconda3/envs/chaya_test/lib/python3.6/site-packages/openforcefield/typing/engines/smirnoff/forcefield.py", line 1207, in _check_for_missing_valence_terms
    raise Exception(msg)
Exception: HarmonicBondForce: Mismatch between valence terms added and topological terms expected.
Topological atom sets not assigned parameters:
                        (1, 2) :     0      C2     0      C3
                        (0, 1) :     0      C1     0      C2
                        (6, 7) :     0      C7     0      C8
                       (3, 13) :     0      C4     0      H5
                        (5, 6) :     0      C6     0      C7
                        (4, 5) :     0      C5     0      C6
                       (4, 14) :     0      C5     0      H6
                        (1, 6) :     0      C2     0      C7
                       (2, 12) :     0      C3     0      H4
                        (2, 3) :     0      C3     0      C4
                        (3, 4) :     0      C4     0      C5
                        (5, 8) :     0      C6     0      C9
topology_set:
{(5, 6), (8, 19), (1, 6), (0, 10), (2, 12), (7, 16), (5, 8), (1, 2), (6, 7), (7, 15), (8, 20), (0, 11), (3, 13), (4, 5), (4, 14), (2, 3), (0, 1), (8, 18), (7, 17), (0, 9), (3, 4)}
assigned_set:
{(7, 15), (8, 20), (0, 11), (8, 19), (7, 16), (0, 10), (8, 18), (7, 17), (0, 9)}

If I use the direct path to the github version of Frosst_AlkEthOH_parmAtFrosst.offxml with the script below there are no problems:

smiles = 'Cc1cccc(c1C)C'
new_mol = oechem.OEMol()
oechem.OESmilesToMol(new_mol, smiles)
oechem.OEAddExplicitHydrogens(new_mol)
# using complete path to my github cloned openforcefield package
FF_filename = "~/github/openforcefield/openforcefield/data/forcefield/Frosst_AlkEthOH_parmAtFrosst.offxml')
ff = forcefield.ForceField(FF_filename)
top, syst, pos  = ff_utils.create_system_from_molecule(ff, new_mol, verbose = False)

Also, if you use FF_filename = get_data_filename('forcefield/SMIRNOFF99Frosst.offxml') there is no problem. We need to make sure that

1) All offxml files in github and the conda versions are useable and up to date with whatever the current file format is.
2) Every example in our READMEs can be run as written!

@ChayaSt @j-wags @jchodera @davidlmobley Here's the details of what we were posting about on #smirnoff today.

[j-wags]

I don't think this is related to the problem, but it's reminded me that the topology branch has started modifying the SMIRNOFF spec a bit (doing things like adding a ToolkitAM1BCC tag, and changing the NonBondedForce/vdW cutoff attrib from a format like 'cutoff' = '8.0*angstrom' to 'cutoff_unit'='angstrom', 'cutoff' = 8.0)

Example of old spec: https://github.com/openforcefield/openforcefield/blob/master/openforcefield/data/forcefield/Frosst_AlkEthOH_parmAtFrosst.offxml#L48
Example of new spec: https://github.com/openforcefield/openforcefield/blob/topology/openforcefield/data/forcefield/smirnoff99Frosst.offxml#L307

This means that, when we merge the topology branch into master, we'll need to replicate the history of changes to the offxml files in master, but in accordance with the new spec.

[j-wags]

@bannanc, which bad offxml file was it finding before you debugged it? Was the bad file bundled in the conda package, or was it picking up a file from a different installation/source?

[jchodera]

Is it clear that this is mixing new (SMIRNOFF 1.0) and old (SMIRNOFF 0.1) specs?

[j-wags]

At a glance, I don't think this problem is arising from mixing the specs. It just came to mind that offxml files in the master branch are being changed, and at some point we will need to manually merge the topology branch back in, adding those changes. I'll update my previous post to clarify.

[bannanc]

@j-wags I didn't double check this, but I have to believe the bad file is in the conda installation because it happened in clean conda environments on mine and Chaya's computers.

I'm pretty sure the file, Frosst_AlkEthOH_parmAtFrosst.offxml, was created so we could reproduce bugs in parm@Frosst with the SMIRNOFF format early on when we were verifying that the format was working the way we wanted. So this file isn't necessarily that important.

The bigger problem here is that we need to make sure examples and code snippets in README's can be ran as written, especially in conda and stable releases. I found an issue in one of the examples as well, I'll post a separate issue for that one, but we really need to be checking examples and snippets.

[jchodera]

Overhauling all the examples is a big job, though important!

It would be awesome if there was a way we can automate testing of all the examples we provide.

[bannanc]

I know its a big job, but at the least the first example in our home README should work, which is what Chaya was using and it doesn't.

[bannanc]

We can fix that specific example by switching it to smirnoff99Frosst instead of Frosst_AlkEthOH_parmAtFrosst.offxml. If we want to call that good enough for now, I can put in a PR switching that.

[j-wags]

The bigger problem here is that we need to make sure examples and code snippets in README's can be ran as written, especially in conda and stable releases.

@andrrizzi and I have two weeks planned where we'll primarily be polishing docs and testing examples in preparation for the alpha 1.0.0 release. However I think the 1.0.0 API is sufficiently different that the fixes we make there won't overlap much to the 0.0.X toolkit examples.

It would be awesome if there was a way we can automate testing of all the examples we provide.

_"Objection, your honor, the opposing counsel is begging the question"_

There's a module that does just that (I think @jchodera actually told me about it): https://docs.python.org/3/library/doctest.html . I'll open an issue to get it integrated with our testing framework.

[bannanc]

nbval will also test jupyter notebooks, so if we can get the examples in the examples folder primarily into jupyter notebooks we can use that.

I'm using it to test the jupyter notebooks in chemper and then integrating those directly into my ReadTheDocs as examples so you don't have to write the code out separately.

[j-wags]

Additional task for this: https://github.com/openforcefield/openforcefield/pull/215#pullrequestreview-220698906

Once finalized we are also going to need to change the template files in utilities/convert_frosst to reflect new syntax, versionining, date, etc., then use this to re-generate smirnoff99Frosst in the correct format (basically it's generated from a smirnoffishFrcmod.parm99Frosst.txt file via convert_frcmod.py there which puts it into the appropriate XML format, basically for historical reasons -- in porting it, Chris didn't like working with the XML (understandably) and so dealt with an AMBER-style file resembling a frcmod file but with SMIRKS provided in the first column rather than an atom type; the convert_frcmod.py file puts this into the correct XML format).

[bannanc]

To help whoever is working on this here are the sections of convert_frosst.

This is a function for adding the date and authors called add_date_and_author, this could be updated to include versions.

Then the main function convert_frcmod_to_ffxml uses functions Forcefield.addParameter, Forcefield.getParameters, and Forcefield.setParameter.

The bulk of convert_frcmod.py is just bookkeeping and shouldn't need to change.