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

TorchEnvironmentProvider #101

Merged
merged 6 commits into from
Jun 17, 2019
Merged

Conversation

Dom1L
Copy link
Contributor

@Dom1L Dom1L commented Apr 24, 2019

Hi,

this PR contains an environment provider based on the neighborpair implementation of TorchANI. I was talking about it in issue #96. It can be used either on a CPU or GPU and provides a speedup compared to the ASE implementation (tested on a TitanV with a 352 atom structure):

  • 16.3 ms Torch
  • 1.12 s ASE
    The switch to numpy after the calculation of the indices does not seem to bother the speed, as I also tried a Torch version of this.

A downside is that it requires a fairly new Pytorch version that contains new functions (for example cartesian products). I used Pytorch 1.1.0.dev20190411 for this one. So one would need to add that to the requirements to pass the tests. I didn't do it because I wasn't sure how you want to handle that.

I also added the TorchEnvironmentProvider to the existing tests for the environments and made sure it passes them. However, the returned neighborlists are sorted in contrast to the ASE ones, so that might trigger some asserts when testing with larger structures. I am also not an expert with neighborlists, so I'm not sure whether the different Torch and ASE implementations cause any problems. But as it is right now, it might be a nice option to have.

Hope it helps and if there is anything else let me know

@ktschuett
Copy link
Contributor

We have to figure out how to do this with the torch version. Do you know when this will be released? Perhaps, we hold this back until then...

@Dom1L
Copy link
Contributor Author

Dom1L commented Apr 26, 2019

I totally agree, this was also my biggest concern.
A quick solution would be to just avoid these new functions, but I think holding it until the 1.1 release is completely fine, as this is not an urgent feature. I just don't know when it's coming though...

@ktschuett ktschuett changed the base branch from master to dev June 17, 2019 10:18
@ktschuett ktschuett merged commit f480b98 into atomistic-machine-learning:dev Jun 17, 2019
ktschuett pushed a commit that referenced this pull request Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants