Add simpler version of parameter swapping example notebook that uses openmm-forcefields

[jchodera]

This PR adds a much simpler, alternative version of the parameter swapping example that uses the openmm-forcefields package, which provides SMIRNOFF support for OpenMM ForceField via a SMIRNOFFTemplateGenerator.

However, this won’t work until we have fixed this issue with the BRD4 complex PDB files lacking CONECT records:
MobleyLab/benchmarksets#70

• [] Tag issue being addressed
• Add tests
• Update docstrings/documentation, if applicable
• Update changelog

[davidlmobley]

This looks like a valuable example. Initially I misread this as saying it was going to replace the other example, so I was going to argue that was a bad idea -- but it looks like this just adds an additional example, and it's a valuable one.

[jchodera]

We can always consolidate later, but for now, deployment of openmm-forcefields requires a couple of extra human steps.

[jchodera]

Now that the PDB files have been fixed by MobleyLab/benchmarksets#71, I have completed this notebook example.

You can view the notebook here.

[jchodera]

@j-wags : I'll update travis to fix these failing tests. I just have to add a few lines to install openmm-forcefields following this diff.

[jchodera]

Phew! Looks like I fixed the tests, @j-wags.

[jchodera]

@j-wags : Weird, the openeye travis tests are failing:
https://travis-ci.org/openforcefield/openforcefield/jobs/637488606#L1273-L1406

Any ideas what might be going on?

[jchodera]

Oh, this is because I foolishly installed openforcefield and openforcefields again! Let me fix.

[jchodera]

OK, I'm stumped now.

[j-wags]

Hm, this may be something deeper. I'll take a look at this today (for real this time)

[j-wags]

It looks like an old commit from November got caught up in this branch, introducing a SMILES roundtrip test. That was failing because our test dataset (some subset of DrugBank) has a couple weird 3D strictures that don't define some stereocenters. Other tests had encountered this problem and had sets of known "stereochemically bad" molecules. I've consolidated all of these "stereochemically bad" molecules for each toolkit into one place and added logic to prevent these from causing failures in the SMILES roundtrip test. I'm also now running the SMILES roundtrip tests forcing the use of each toolkit (so, it runs the test set once with RDKit and then again with OETK).

I'm hoping that this will get tests passing. I'll review the notebook in here tomorrow morning.

[jchodera]

@j-wags: Just a reminder that this still needs to be merged.

[j-wags]

@jchodera I think the notebook itself is in OK shape to merge, but this will fail on any computer without the OpenMM patch. That seems pretty bad, since the average user won't know to look in our travis.yml to find the patch, and we don't document it. Is there an urgent need to release this? If so I can add some patch instructions to the notebook, and we'll just need to make sure we revert the build system and notebook when OpenMM 7.4.2 comes out.

[codecov-io]

Codecov Report

Merging #486 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

[j-wags]

@jthorton and I found that this patch actually breaks conda, since the OpenMM installation is common to several environments, so conda raises a SafetyError because it considers the patch-modified OpenMM installation to be corrupt.

[jchodera]

Note that the openmm nightly dev snapshots include the patch. We can install that via conda with

conda install --yes -c omnia-dev openmm

In fact, you don't need the patch as long as you're only using GAFF, so we could just test this notebook only on a travis branch that uses omnia-dev to make sure everything works against the development release.

We have the SMIRNOFF torsion support in the release roadmap for OpenMM 7.5.0, which we're hoping to release shortly.

[jchodera]

@j-wags : What should we do to clean this up and get it merged? The example will still work with the omnia-dev version of OpenMM until OpenMM 7.5 is released---perhaps we could just warn about that?

[j-wags]

Hm, so the solutions I can think of here are either:

1, the current solution) we apply a patch in testing that doesn't reflect any main-label conda packages, complicates our build system, and borks multiple conda environments should a user ever find and apply it. We add a note that the example shouldn't be used without OMM>=7.5, and add instructions about getting a nightly build of OMM, with instructions on how to do so. We create our 183rd open Issue saying "remember to undo this patch when OMM7.5 comes out".
2) we add another set of travis builds to test with OMM nightlies, adding a new pytest mark for tests/notebooks which require nightlies, which we'd need to review and remove each time there's an OMM release. This will increase our already-substantial build time, introduce complexity, and require constant human intervention to keep our testing framework out of a "fail-deadly" state. The success or failure of "nightly" tests would randomly occur simply due to moments when OMM master was in a weird state, so we'd learn to ignore it.
3) we switch all of our builds over to OMM nightlies, which may cause them all to fail if OMM master isn't in a good state (and AFAIK OMM makes no guarantee that master be in a passing state at any moment). Also, if there's a reverse-incompatible change coming up, it would cause all of our tests to fail, even if the reverse-incompatible changes weren't deployed when we wanted to do our own release.
4) we merge this notebook, add a warning that it needs OMM 7.5, and --ignore it in pytest. We add our 183rd open issue: "remember to un-ignore this test when OMM 7.5 is released"
5) we don't merge this notebook until OMM 7.5 is released

Manually applying the patch (option 1) is risky -- Josh and I naively applied the patch to our dev environments, and it borked several conda environments and took us a while to figure out what had happened, so that leaves a bad taste in my mouth.

I don't think we need to move heaven and earth for this PR, since OMM 7.5 will be out in a few weeks, and my plate is full with high-demand PRs like #281, #471, and WBO torsions (not begun), all of which were due today. So options 2 and 3 are probably not appropriate, unless we anticipate needing bleeding-edge features from OMM regularly. If that is the case, adding a nightly build matrix is a moderate-effort development, and it should be prioritized against other developments, especially since it's nontrivial to understand what to do with a PR if it passes tests based on nightly builds but fails on stable, or vice versa.

If this needs to be merged before the OMM 7.5 release, option 4 (just merge but exclude from tests) seems like the best option . But otherwise option 5 (wait for OMM 7.5) is by far the simplest.

[jchodera]

@j-wags : OpenMM 7.4.2 is out now, so this can be updated and merged.

We may want to just eliminate the old, complicated example and just use the _with_openforcefields.ipynb version.

[j-wags]

Ahhh, ok. This test failure just "clicked" for me.

In the 0.6.0 OpenFF toolkit, Molecule.from_file couldn't distinguish between a loaded file:

• having explicitly all-zero charges (like, a mol2 with all 0.000 partial charges), versus
• a file not having charges defined at all, like an SDF before we supported partial charges.

The 0.7.0 release will have the machinery to tell the difference. Files which don't have partial charges defined are loaded with mol.partial_charges = None, while those with all-zero charges are loaded with mol.partial_charges = np.zeros([mol.n_particles]).

Unfortunately, openmmforcefields can't handle a molecule where mol.partial_charges = None. The issue is with comparing different-sized arrays at this line.

To fix, I'll:

1. open a PR to openmmforcefields to handle either case, and
2. update the notebook in the PR to convert partial_charges values of None to zeros

We can roll back 2) next time there's a release of openmmforcefields that includes the PR from 1).

[codecov-commenter]

Codecov Report

Merging #486 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

[jchodera]

Unfortunately, openmmforcefields can't handle a molecule where mol.partial_charges = None.

Should it? If it should, I can cut a new release today if you open an issue and mark it as urgent!

[j-wags]

Opening a openmmforcefields PR shortly with a patch. Right now I don't think there are many ways to create a molecule with partial_charges=None, but after the 0.7.0 toolkit release (ETA end of this week), there will be a lot more ways to get that. So an openmmforcefields release this week would lead to a seamless toolkit update.

[j-wags]

Awesome. Merging!

[jchodera]

Thanks!

[jchodera]

Also, once we address #488, this won't be an issue!