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

Refactor: Unify the interfaces of Pulay terms of force and stress #5130

Merged
merged 8 commits into from
Oct 16, 2024

Conversation

maki49
Copy link
Collaborator

@maki49 maki49 commented Sep 18, 2024

Why need this change

To implement the analytical gradient of LR-TDDFT, we need to calculate:
image

where $(T+D^{Z})$ is a density matrix different from the ground-state (GS) one, and $\Lambda$ is a complicated energy density matrix different from the GS one.

However in the current code, the GS energy density matrix is hard-coded in cal_fedm, blocking me from implementing the feature.

Additionally, I notice that if the energy density matrix is treated equally as the density matrix as the input argument, the Pulay terms of force can be abstracted to $\text{Tr}[D\frac{dH}{dr_x}]$, and similarly $\frac{1}{\Omega}\text{Tr}[D\frac{dH}{dr_x}r_y]$ for stress, which can avoid a lot of duplicate code (cal_fedm, cal_ftvnl_dphi and cal_fvnl_dphi).

What's changed

Unify cal_fedm, cal_ftvnl_dphi and cal_fvnl_dphi (both gamma-only and multi-k) into a new function cal_pulay_fs , where 3 overloads are provided:

  • calculated by 2-center integration, provided dH(x, y, z) and dH*r(11, 12, 13, 21, 22, 23)
  • calculated by 2-center integration, provided dH(x, y, z) and dr(x,y,z)
  • calculated by grid integration

All of them above treat the (energy) density matrix as an input parameter, and the Hamiltonian term (any operator) is passed in the way according to the algorithm.

source/Makefile.Objects Outdated Show resolved Hide resolved
@dyzheng
Copy link
Collaborator

dyzheng commented Sep 18, 2024

The refactor of force or stress code should avoid the memory cost of $$\frac{dH}{d\tau_i}$$ for force or $$\frac{dH}{d\tau_\alpha}\tau_\beta$$ for stress, otherwise it will be refactored again in the future.

@maki49
Copy link
Collaborator Author

maki49 commented Sep 18, 2024

The refactor of force or stress code should avoid the memory cost of d H d τ i for force or d H d τ α τ β for stress, otherwise it will be refactored again in the future.

You can delete those arrays in fsr later. It does not conflict with my demands.

@mohanchen
Copy link
Collaborator

You can delete those arrays in fsr later

In PRs, try to fix the issues proposed by others, don‘t let others do anything to meet your demands. Nobody has the obligation to do something to meet your personal demands. You need to consider more for others.

@maki49
Copy link
Collaborator Author

maki49 commented Sep 19, 2024

In PRs, try to fix the issues proposed by others, don‘t let others do anything to meet your demands. Nobody has the obligation to do something to meet your personal demands. You need to consider more for others.

I didn't let others do anything to meet my demands. I'm meeting my demands by myself.
Deleting fsr (already existing, not my addition) is another plan of @dyzheng. We've talked about it.
The two things do not conflict.

@maki49 maki49 force-pushed the pulay_fs branch 3 times, most recently from b0113a1 to 208c057 Compare September 21, 2024 11:12
@mohanchen mohanchen merged commit 2ddef0c into deepmodeling:develop Oct 16, 2024
14 checks passed
@mohanchen mohanchen added the Features Needed The features are indeed needed, and developers should have sophisticated knowledge label Oct 16, 2024
@maki49 maki49 deleted the pulay_fs branch October 22, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Features Needed The features are indeed needed, and developers should have sophisticated knowledge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants