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

Add generate_kvectors cache #7

Merged
merged 4 commits into from
Dec 22, 2023
Merged

Add generate_kvectors cache #7

merged 4 commits into from
Dec 22, 2023

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented Dec 7, 2023

The cache is used within the compute method of FourierSpaceConvolution and is applied when the cell and the number of mesh points has not change compared to the previous call.

However, I did some little tests and the speedup was very minor (~10%). Are you sure @kvhuguenin that generate_kvectors is really responsible for 30% of the total computation time in MeshPotential?

I also renamed smearing to atomic_smearing to consistent within the library and added an example for the usage of FourierSpaceConvolution.


📚 Documentation preview 📚: https://meshlode--7.org.readthedocs.build/en/7/

@kvhuguenin
Copy link
Contributor

I have done more systematic tests now. TLDR: everything seems to behave the way it should.

The results are that the savings in computational cost are on the order of 30% without gradients and 15% with gradients for structures with system sizes on the order of N=1e2 to N=1e4. Some comments:

  • Firstly, the fact that we save 30% without gradients matches the results from the profiling I had done two weeks ago.
  • Furthermore, the fact that we only save half as much with gradients is consistent with the fact that for the backpropagation, all the FFT machinery (the dominant part of the cost) is now run twice, so the relative fraction of the k-vector generation is smaller.
  • Also, for structures with N<10, the savings in computational cost are much smaller, most likely because other "constant" contributions in the cost become much more significant.

In summary, the behavior of the caching seems to be exactly what we expect. I just tried to improve the documentation a little, but otherwise, this seems a good addition to the main branch.

@kvhuguenin kvhuguenin merged commit d915d0f into main Dec 22, 2023
4 checks passed
@PicoCentauri PicoCentauri deleted the kvec_cache branch February 29, 2024 16:39
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