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

Implement caching for Clebsch-Gordan matrix construction. #37

Merged
merged 6 commits into from
Jun 3, 2024

Conversation

YCC-ProjBackups
Copy link
Contributor

This PR only contains the caching of the CG matrix. As Arthur and I discussed, we decided that it is better to split Rust port PR and caching PR.

Please refer to the AniSOAP paper, appendix A-9-a for the implementation details.

@cbefan
Copy link

cbefan commented May 31, 2024

This works well and is certainly better than no caching, but I'm not sure if this memory paging will be relevant for AniSOAP™ applications, given that l_max will generally be held constant? It seems like we could just cache the matrix.

@arthur-lin1027
Copy link
Collaborator

This works well and is certainly better than no caching, but I'm not sure if this memory paging will be relevant for AniSOAP™ applications, given that l_max will generally be held constant? It seems like we could just cache the matrix.

@cbefan It seems that the question is whether we want the user of AniSOAP to cache the CG matrix or if we want the matrix to be cached internally. Making the user have to worry about this component makes the code less user friendly.

@cbefan
Copy link

cbefan commented Jun 3, 2024

@arthur-lin1027 I agree it should be stored internally, but we should just be able to cache the matrices internally. The memory paging seems a little gratuitous when the handful of matrices themselves can just be internally cached without any apparent issue.

@arthur-lin1027
Copy link
Collaborator

Agreed it's that this paging scheme is more than necessary but since it's there and it seems to work, I think we can keep it, unless it severely obfuscates the code (most people probably won't see the inner workings of this so I think it's fine. If it turns out to be an issue we can easily remove it later.

Copy link

@cbefan cbefan left a comment

Choose a reason for hiding this comment

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

Looks good! Maybe be careful about hardcoding the cache list length to five, but it should be alright for the forseeable future.

@arthur-lin1027 arthur-lin1027 merged commit dc1a1dc into cersonsky-lab:main Jun 3, 2024
4 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.

3 participants