-
Notifications
You must be signed in to change notification settings - Fork 145
Fix the overwriting between xc kernel components #5654
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
Conversation
dyzheng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes in this PR are relatively simple and clear, with a style consistent with the module_lr module. While there is a difference from the main code style of other modules, this part of the code is relatively independent, and therefore I still recommend accepting it. Suggestions for developers include: 1. memory usage should be as cautious as possible, and manual release should be performed promptly after use. 2. There are multiple occurrences of OMP parallel divisions in the code; since each allocation of OMP tasks requires time, it might be worth considering reducing the number of OMP divisions.
kirk0830
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because files changed in this PR requires the knowledge of libxc, in which many quantities are defined with names commonly seen in libxc, like sigma, ..., So I prone not leave quite many comments on variables' name.
There is one minor suggestion:
I notice you have many simple numerical operations that can be omp-paralleized, a better way to organize your codes can be write several functions like:
void _customize_op1(const std::vector&, ..., ...);in those functions you implement conditional omp threading, and follow the present coding regulation that do not write for-loop in one-line manner (because human reads line from the left to right, in your code the control of for-loop/bound will be the first to read, which is not the most important information of one for-loop block, instead, the vectorized data being iterated on, and operations are two the most important information. This is the essence of functions like std::transform, std::for_each in aspect of code-readability, as far as I understand) to improve the code readability:
Do not write a for-loop like:
for (int i = 0; i < bound; i++) {/* do something */}Instead, your write like:
for (int i = 0; i < bound; i++)
{
/* the thing really important */
}* fix the overwriting between xc kernel components * [pre-commit.ci lite] apply automatic fixes * use for-loop with omp * add duration test * replace lambda by individual function * expand the for-loop * small change --------- Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Fix the problem that
XC_LDA_C_PZoverwritesXC_LDA_Xin LR-TDDFT.