Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing the dependency of TorchANI #44

Open
raimis opened this issue Dec 10, 2021 · 2 comments
Open

Removing the dependency of TorchANI #44

raimis opened this issue Dec 10, 2021 · 2 comments
Labels
enhancement New feature or request

Comments

@raimis
Copy link
Contributor

raimis commented Dec 10, 2021

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

@peastman proposal (#39 (comment)):

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 raimis added this to Regular in Accelerated NNP in OpenMM via automation Dec 10, 2021
@raimis
Copy link
Contributor Author

raimis commented Dec 10, 2021

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

raimis pushed a commit to raimis/NNPOps that referenced this issue Dec 10, 2021
raimis pushed a commit to raimis/NNPOps that referenced this issue Dec 10, 2021
@raimis
Copy link
Contributor Author

raimis commented Dec 16, 2021

On Tuesday, we decided:

raimis pushed a commit that referenced this issue Dec 20, 2021
* Optimize EnergyShifter of TorchANI

* Install EnergyShifter and enable testing

* Copy the model to a correct device

* Update READM.md

* Fix typo

* Simplify code

* Simplify more

* Remove the TorchANI import from the top level (#44)
raimis pushed a commit that referenced this issue Dec 20, 2021
* Implement TorchANISpeciesConverter

* Add test for TorchANISpeciesConverter

* Update README.md

* Remove the TorchANI import from the top level (#44)
raimis pushed a commit to raimis/NNPOps that referenced this issue Dec 20, 2021
@raimis raimis mentioned this issue Dec 20, 2021
3 tasks
raimis pushed a commit that referenced this issue Dec 21, 2021
* Optimize EnergyShifter of TorchANI

* Install EnergyShifter and enable testing

* Copy the model to a correct device

* Update READM.md

* Fix typo

* Simplify code

* Simplify more

* Implement TorchANISpeciesConverter

* Add test for TorchANISpeciesConverter

* Update README.md

* Implement OptimizedTorchANI

* Add tests for OptimizeTorchANI

* Import directly OptimizedTorchANI

* Fix a file name

* Add an example for OptimizedTorchANI

* Fix the test tolerances

* Fix a type in the example

* Move the top-level import of TorchANI

* Update the doc string (#44)
@raimis raimis moved this from Regular to Long-term in Accelerated NNP in OpenMM Mar 5, 2022
@raimis raimis added the enhancement New feature or request label May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

No branches or pull requests

1 participant