Closed ipcamit closed 6 months ago
PS: @mjwen I am also tagging Eric (@EFuem ) in this PR as he has offered to be an extra set of eyes for helping out in the transition.
This is great. Thanks @EFuem !
@ipcamit thanks!
I don't think there will be major changes needed but a couple of small tweaks. I have no idea of how KLIFFTorchGraph
is going to be used and thus weren't able to provide feedback.
Also, let's be consistent in naming and such:
pyg
or PyG
in function/class names for torch_geometric related stuff?KIMDriver
for the graphs and related stuff for your driver?Ok, let me go through the comments and also add explanatory note on graph utilities and classes. In brief, KLIFFTorchGraph is torch geometric compatible graph generated such that it is completely local and works with OpenKIM drivers. Its biggest benefit is that it does not require any global lattice information etc, and can be used for parallel graph convolutions. Would add more information with first round of improvements.
@ipcamit thanks!
I'll give it a second go soon and provide feedback if I have.
@ipcamit A few more comments. The only major one is on the use of NeighborList
in Descriptor
. We can discuss further.
sure. let me go through these and then we can discuss further.
I have commented in detail above. The main use case is to avoid any conditional imports of the dataclass. Without torch geometric it will log a warning and work as a simple data container. With torch geometric it will also collate. So with or without torch geometric KLIFF code will work. With Torch geometric it will be just more efficient.
All good. I think the only missing part is type annotation. Can you please add that?
I have added type annotations wherever I could. Please let me know if you spot any missed ones.
Summary
This PR integrates all the transforms for the Configuration objects, including configuration transforms (fingerprinting), and property transforms (normalization of energy and forces). The coordinate transforms are the major contribution which includes graph generation and descriptor generation from
libdescriptor
. It also introduces functions to compute some descriptor widths inkliff.utils
as I was not sure of where to best keep those functions. I will commit the unit tests for these modules next, which will install the libdescriptor python package to compute and compare descriptors. That commit will also move existing descriptor module to legacy. But will do all of that in next commit with first round of comments.Also, with this commit I have modified the README file to state the purpose of this branch, as Stefano asked it for pointing it to people. Please let me know if we should include more information.
PS: @mjwen I am also tagging Eric (@EFuem ) in this PR as he has offered to be an extra set of eyes for helping out in the transition.