Changed default charge method to nn.

[madilynpaul]

This pull request is to fix issue #141.

The charge method default was set to 'am1-bcc' rather than 'nn'. This has been changed in this PR.

[mikemhenry]

Thanks for the PR @madilynpaul

[mikemhenry]

@jchodera @yuanqing-wang thoughts on this?

[mikemhenry]

@jchodera @yuanqing-wang thoughts on this?

[codecov-commenter]

Codecov Report

Merging #142 (2069640) into master (63ae6c6) will not change coverage.
The diff coverage is n/a.

[jchodera]

I believe we do want to use nn charges by default, but isn't this already addressed in the updates for espaloma-0.3.0?
cc @kntkb

[kntkb]

I think it's reasonable to use nn as default charges but it's still not updated in espaloma 0.3.0 release. But it should be kept in mind that if espaloma is not trained with partial charges or serialized with partial charge prediction, then it will fail to assign partial charges with nn.

[jchodera]

@yuanqing-wang : What do you think?

[yuanqing-wang]

Yes. Let's use NN as our default charge method!

[yuanqing-wang]

But it should be kept in mind that if espaloma is not trained with partial charges or serialized with partial charge prediction, then it will fail to assign partial charges with nn.

I think we don't need to go back to the old version once we release the stable version of 0.3.0

[ijpulidos]

I think we don't need to go back to the old version once we release the stable version of 0.3.0

Do we expect people to retrain espaloma without partial charges? Or do we expect having some serialized espaloma model without them? If that's the case, then deploying with the default charge method (nn) would fail, correct?

[jchodera]

I'd expect the use cases are likely to be (in order of popularity):

1. Using espaloma-0.3.0 model directly
2. Tweaking or retraining espaloma-0.3.0 with some new data
3. Retraining an espaloma model from scratch

So we should focus on the first couple of use cases.

Perhaps we could offer folks the option to use AM1-BCC charges if they want, but they may also want to use the charges in the provided Molecule objects instead. We shouldn't prefer one.

If we can throw an intelligent exception of the model lacks the parameters or readout needed to assign charges, that would be even better, but this is a refinement we can leave until later.

[kntkb]

Espaloma is already designed so that the users could optionally specify a charge method.I think this PR is ready to go.

[kntkb]

We're failing the test because the graph molecule is not parameterized with an espaloma model. We need to add or revise the test so that the molecule graph is passed to an espaloma model before calling esp.graphs.deploy.openmm_system_from_graph.

We could do something like the following:

import urllib
version="0.2.2"
url = f'https://github.com/choderalab/espaloma/releases/download/{version}/espaloma-{version}.pt'
cached_filename = f'espaloma-{version}.pt'
urllib.request.urlretrieve(url, filename=cached_filename)

def test_butane():
    g = esp.Graph("CCCC")
    ff = esp.graphs.legacy_force_field.LegacyForceField("openff-1.2.0")
    g = ff.parametrize(g)
    # apply a trained espaloma model to assign parameters
    net = torch.load(espaloma_model_filepath,  map_location=torch.device('cpu'))
    net.eval()
    net(g.heterograph)
    esp.graphs.deploy.openmm_system_from_graph(g, suffix="_ref")

Oh but we need a model >0.3.x