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

getNeighborPairs compute the displacement vector #61

Merged
merged 10 commits into from
Jul 21, 2022

Conversation

raimis
Copy link
Contributor

@raimis raimis commented Jul 7, 2022

  • Compute the vector
    • CPU
      • Forward pass
    • CUDA
      • Forward pass
      • Backward pass
  • Update tests
  • Update documentation

@raimis raimis self-assigned this Jul 7, 2022
@raimis raimis mentioned this pull request Jul 7, 2022
@raimis raimis marked this pull request as ready for review July 8, 2022 14:15
@raimis raimis requested a review from peastman July 8, 2022 14:15
@raimis
Copy link
Contributor Author

raimis commented Jul 8, 2022

@peastman can you review?

@peastman
Copy link
Member

peastman commented Jul 8, 2022

test_neighbor_grads() checks that the CPU and CUDA versions give the same results, but it doesn't check that they're actually correct. How about adding a test for correctness? For distances and deltas, it's easy to compute analytically what the expected gradients are.

@raimis
Copy link
Contributor Author

raimis commented Jul 11, 2022

@davkovacs I have built a conda package from the latest commit (32eeae5).

conda install -c conda-forge -c raimis/label/pr61 nnpops==0.3rc0

Let me know if it works as you wished and I'll finish the PR.

@davkovacs
Copy link

davkovacs commented Jul 12, 2022

Would it work with PyTorch 1.12? We want to compile our models with torchscript to create the TorchForce object, but have some operations that can only be compiled with the latest PyTorch.

@raimis
Copy link
Contributor Author

raimis commented Jul 12, 2022

At the moment, conda-forge pins to PyTorch 1.11 (https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/ed6ced1ec023a07826211329f97bcf75466fdc97/recipe/conda_build_config.yaml#L728).

If you want PyTorch 1.12, you have to build from the source.

@davkovacs
Copy link

I am going on holiday now, so I will not be able to test this thoroughly for the next couple of weeks. But will be one of the first things when I am back.

@raimis
Copy link
Contributor Author

raimis commented Jul 19, 2022

conda-forge started to migrate to PyTorch 1.12.

@raimis
Copy link
Contributor Author

raimis commented Jul 19, 2022

test_neighbor_grads() checks that the CPU and CUDA versions give the same results, but it doesn't check that they're actually correct. How about adding a test for correctness? For distances and deltas, it's easy to compute analytically what the expected gradients are.

For CUDA, I write the backward pass explicitly. For CPU, the backward pass is implicit and relies on the autograd. So, two independent implementation of gradients agree. This should be enough to prove correctness.

@peastman
Copy link
Member

Ok, that makes sense.

@raimis raimis merged commit 16ca7e4 into openmm:master Jul 21, 2022
@raimis
Copy link
Contributor Author

raimis commented Jul 21, 2022

@davkovacs conda-forge started to migrate to PyTorch 1.12. There will be new packages, when the dependencies migrated (conda-forge/nnpops-feedstock#9)

@raimis raimis mentioned this pull request Dec 7, 2022
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