Skip to content

Add cuML KNN for graph construction, better handling of non-PBC systems#69

Merged
vsimkus merged 2 commits intoupdate-v3from
update-v3-cuml
Mar 20, 2025
Merged

Add cuML KNN for graph construction, better handling of non-PBC systems#69
vsimkus merged 2 commits intoupdate-v3from
update-v3-cuml

Conversation

@vsimkus
Copy link
Copy Markdown
Contributor

@vsimkus vsimkus commented Mar 20, 2025

  • cuML for KNN graph construction
  • Determining if a system is periodic using ase.Atoms.pbc and ase.Atoms.cell rather than just ase.Atoms.cell.

I've added a note to the README regarding optionally installing cuML.

I've tried adding cuml as an optional dependency in poetry such that users can pip install orb-models[cuml-cu11] or pip install orb-materials[cuml-cu12] depending on their system. But poetry tries to resolve dependencies between the two optional packages cuml-cu11 (for cuda 11) and cuml-cu12 (for cuda 12) and fails since they require different sub-dependencies. I don't think there is currently a way to specify mutually-exclusive optional dependencies in pip/poetry right now. So it's probably best to just have it in the README.

@vsimkus
Copy link
Copy Markdown
Contributor Author

vsimkus commented Mar 20, 2025

On that note, should we perhaps set the default edge_method in Calculator to None:

edge_method: Optional[EdgeCreationMethod] = "knn_scipy",

such that the default method would be chosen dynamically:
def get_default_edge_method(
device: torch.device, num_atoms: int, is_periodic: bool
) -> EdgeCreationMethod:
"""Get the default edge method for a given device and number of atoms."""
if device.type != "cpu":
if (
cuml is None
or (is_periodic and num_atoms < 5_000)
or (not is_periodic and num_atoms < 30_000)
):
edge_method = "knn_brute_force"
else:
edge_method = "knn_cuml_rbc"
else:
edge_method = "knn_scipy"
assert edge_method in typing.get_args(EdgeCreationMethod)
return edge_method # type: ignore

Copy link
Copy Markdown
Contributor

@DeNeutoy DeNeutoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super, LGTM!

@vsimkus vsimkus merged commit cb25856 into update-v3 Mar 20, 2025
6 checks passed
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.

2 participants