Skip to content

Conversation

@Luthaf
Copy link
Contributor

@Luthaf Luthaf commented Apr 16, 2025

Summary

Fix #155

I don't have access to a CUDA GPU, so I would appreciate either @frostedoyster or @orionarcher checking that this does work in practice!

Checklist

Before a pull request can be merged, the following items must be checked:

  • Doc strings have been added in the Google docstring format.
    Run ruff on your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

Note that the CI system will run all the above checks. But it will be much more
efficient if you already fix most errors prior to submitting the PR. It is highly
recommended that you use the pre-commit hook provided in the repository. Simply run
pre-commit install and a check will be run prior to allowing commits.

@cla-bot cla-bot bot added the cla-signed Contributor license agreement signed label Apr 16, 2025
@orionarcher
Copy link
Collaborator

orionarcher commented Apr 16, 2025

Works! Thanks for the quick fix.

Question though, is there really no way to run vesin on GPU? I thought that was the role of vesin-torch.

@Luthaf
Copy link
Contributor Author

Luthaf commented Apr 16, 2025

Question though, is there really no way to run vesin on GPU? I thought that was the role of vesin-torch.

Not currently no =) We are working to create a CUDA and/or HIP implementation to make sure the code can run on GPU, hopefully in the next couple of months.

If you want a pure torch implementation of neighbor list, that can run on GPU (because it only uses functions from torch), you can try using https://github.com/felixmusil/torch_nl/, but I found that it is slower than vesin even when running on GPU, except when dealing with very large cells and large (>12A) cutoffs. https://github.com/openmm/NNPOps/ does contain some custom CUDA code for neighbor list, but requires a cell larger than 2 cutoff.

vesin-torch is only exposing the code from vesin in a TorchScript compatible way, and integrating with autograd.

@orionarcher
Copy link
Collaborator

Thanks for the explanation!

Will merge for now and look forward to the upcoming implementation!

@orionarcher orionarcher merged commit 95bba41 into TorchSim:main Apr 16, 2025
90 checks passed
@abhijeetgangan
Copy link
Collaborator

The torch version of neighbor lists which run on GPU are already included in torchsim including an API for torch_nl which accepts the batch as input. https://github.com/Radical-AI/torch-sim/blob/main/examples/scripts/7_Others/7.3_Batched_neighbor_list.py

@orionarcher
Copy link
Collaborator

If that could be subbed in it would be a nice addition to the metatensor model interface!

@Luthaf
Copy link
Contributor Author

Luthaf commented Apr 22, 2025

I'm not sure it is actually worth doing, since in all my tests torch_nl is slower than vesin, even when running on GPU. So unless someone has a system where this is not the case, I would rather keep the code simple!

@janosh janosh added fix Bug fix neighbor list Neighbor analysis labels May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed Contributor license agreement signed fix Bug fix neighbor list Neighbor analysis

Projects

None yet

Development

Successfully merging this pull request may close these issues.

metatensor tests fail on GPU

4 participants