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

Added complete transform module for graph generation and #153

Merged
merged 11 commits into from
Jan 29, 2024

Conversation

ipcamit
Copy link

@ipcamit ipcamit commented Jan 8, 2024

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 in kliff.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.

@mjwen mjwen self-assigned this Jan 8, 2024
@mjwen mjwen added the feature label Jan 8, 2024
@mjwen mjwen self-requested a review January 8, 2024 22:34
@mjwen mjwen removed their assignment Jan 9, 2024
kliff/utils.py Outdated Show resolved Hide resolved
kliff/transforms/property_transforms.py Outdated Show resolved Hide resolved
kliff/transforms/property_transforms.py Outdated Show resolved Hide resolved
@mjwen
Copy link
Collaborator

mjwen commented Jan 9, 2024

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 !

@mjwen
Copy link
Collaborator

mjwen commented Jan 9, 2024

@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:

  • let's use pyg or PyG in function/class names for torch_geometric related stuff?
  • let's use KIMDriver for the graphs and related stuff for your driver?
  • let's remove conditional import as much as possible, which typically can cause a lot of confusion
  • type annotation is needed, but I suppose you plan to do it once everything else is finalized

@ipcamit
Copy link
Author

ipcamit commented Jan 9, 2024

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.

Copy link
Author

@ipcamit ipcamit left a comment

Choose a reason for hiding this comment

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

I think I have addressed the comments, please let me know if you have more suggestions. Pushing the changes after this. Just saw comment about type annotations. Will include it in next iteration.

@mjwen
Copy link
Collaborator

mjwen commented Jan 16, 2024

@ipcamit thanks!

I'll give it a second go soon and provide feedback if I have.

setup.py Show resolved Hide resolved
kliff/transforms/property_transforms.py Outdated Show resolved Hide resolved
kliff/transforms/property_transforms.py Outdated Show resolved Hide resolved
kliff/transforms/property_transforms.py Outdated Show resolved Hide resolved
kliff/transforms/property_transforms.py Outdated Show resolved Hide resolved
kliff/transforms/configuration_transforms/descriptors.py Outdated Show resolved Hide resolved
kliff/transforms/configuration_transforms/descriptors.py Outdated Show resolved Hide resolved
@mjwen
Copy link
Collaborator

mjwen commented Jan 18, 2024

@ipcamit A few more comments. The only major one is on the use of NeighborList in Descriptor. We can discuss further.

@ipcamit
Copy link
Author

ipcamit commented Jan 18, 2024

sure. let me go through these and then we can discuss further.

Copy link
Author

@ipcamit ipcamit left a comment

Choose a reason for hiding this comment

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

Addressed all comments. I think it is nearly done. Will push updated code.

Copy link
Collaborator

@mjwen mjwen left a comment

Choose a reason for hiding this comment

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

All good. The only question I have now is the PyGgraph. I commented in the original thread. Copied here:

else:

    class PyGGraph:

The above defined is not a subclass of pyg data. So, it will not have collate_fn and such. Then what's its use case?

@ipcamit
Copy link
Author

ipcamit commented Jan 28, 2024

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.

@mjwen
Copy link
Collaborator

mjwen commented Jan 28, 2024

All good. I think the only missing part is type annotation. Can you please add that?

@ipcamit
Copy link
Author

ipcamit commented Jan 28, 2024

I have added type annotations wherever I could. Please let me know if you spot any missed ones.

Copy link
Collaborator

@mjwen mjwen left a comment

Choose a reason for hiding this comment

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

  • A couple of annotation needs to be added in __init__
  • nl_ctx in one methods need to be removed

kliff/transforms/property_transforms.py Outdated Show resolved Hide resolved
kliff/transforms/property_transforms.py Outdated Show resolved Hide resolved
kliff/transforms/property_transforms.py Outdated Show resolved Hide resolved
kliff/transforms/configuration_transforms/descriptors.py Outdated Show resolved Hide resolved
@mjwen mjwen merged commit 829d8f7 into openkim:v1 Jan 29, 2024
1 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants