NNPOps icon indicating copy to clipboard operation
NNPOps copied to clipboard

Removing the dependency of TorchANI

Open raimis opened this issue 3 years ago • 2 comments
trafficstars

NNPOps depends on TorchANI but, as discussed in #39, it shouldn't.

@peastman proposal (https://github.com/openmm/NNPOps/pull/39#issuecomment-977020533):

The first thing I would do is open one or more issues on the TorchANI repo suggesting these changes and offering to provide code. This class doesn't implement any kind of calculation. It's just a workaround for a particular flaw in TorchANI (that it repeats the calculation on every step). The more of these changes we can push upstream to them, the better.

For anything they don't want to accept, the next thing I would do is move the imports inside the classes. You should be able to import NNPOps even if TorchANI isn't installed. In practice that means we should never import torchani at the top level. It should only be imported inside the specific classes or functions that use it.

Finally we should consider whether NNPOps is the best place for all of these, or whether, for example, OpenMM-ML would be a better place.

raimis avatar Dec 10 '21 15:12 raimis

As a short-term fix, I'll move import torchani into the classes.

raimis avatar Dec 10 '21 16:12 raimis

On Tuesday, we decided:

  • Move the top level import into classes/functions as a short-term fix -- #45
  • Refactor the wrapper of ANISymmetryFunctions, so it doesn't depend on TorchANI
  • Move the code related to TorchANI into https://github.com/openmm/openmm-ml

raimis avatar Dec 16 '21 11:12 raimis