-
Notifications
You must be signed in to change notification settings - Fork 145
Feature: LR-TDDFT support reading fxc from file or calculating fxc by a specified charge file #5393
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
21a1149 to
8bbbb2f
Compare
32d0022 to
db47c6e
Compare
2e38a44 to
dcee5b3
Compare
7188343 to
97f2591
Compare
Co-authored-by: kirk0830 <[email protected]>
fix compile fix a initialize bug
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.
I have reviewed this PR, and this is the first time that I dive (somehow) into the module_lr code, so I submit several comments on details in files changed, no matter whether they are changes made in this PR.
This PR supports a new functionality that restarting the LR calculation from file that can be provided by user. However, in the view of program design, there are indeed several problems needed to solve in the future.
- file structure is not organized well. In module_lr there is a file named
esolver_lrtd_lcao, and all other files whose names startwithesolverare in foldermodule_esolver. I cannot tell immediately which is a better way but at least the inconsistency is obvious. In module_lr there are folders namedoperator_casida,potentials, andutils. However, in folderoperator_casida, there is a file namedoperator_lr_hxc.cpp, it appears like there is a folder namedfruit_appleand in it there is a file namedplant. There are also files whose names containlr, others containlrtd, so I am confused what is the relationship between thelrandlrtd. I also note there is a a file namedlr_util.hand then anotherlr_util.hpp, this is quite confusing, because.husually indicates header file of C, and.hppfor C++, especially when they appear at the same time. Then there are files namedlr_utiland thenlr_util_xc, there seems to be strong naming overlap:module_lr/utils/lr_util.*. - There is inconsistency in the newly added keyword
lr_init_xc_kernel. The linear-response is abbreviated aslr,initializeasinit, exchange-correlation asxcbut no abbr forkernel. - There is dependency on Global variables such as GlobalC::Pgrid. The problem is other developers cannot realize immediately how to change the behavior of function which is the caller of function where the GlobalC::Pgrid is used as parameter, and if it appears in a class's member function, it is obviously a not good encapsulation.
- The instantiation of class PotHxcLR has explicit dependency on rank number, this design is not clearly explained.
- KernelXC class is used as a place to store data, all its important data members are public. It is explained that because there will be too many getter functions, so no getter function is written.
- The instance of class PotHxcLR will change the global static value XC_Functional::func_id. It is explained that because the module_lr is always the last one called in the whole ABACUS workflow, so this operation should be acceptable. Personally I really doubt about this but seems this is true at present.
@maki49 I suggest you answering to someone who spent a lot of time reviewing your codes in an appropriate way, even if you don't want to follow his suggestions. |
|
@kirk0830 Thanks for spending time to review. For point 5, the public members are all const references being used like getters, as what @mohanchen I have already changed all the things related to the feature implemented in this PR following his suggestion. |
… a specified charge file (deepmodeling#5393) * read fxc or charge for fxc from file * move xc-dependent functions to a new file * minor fix * rename files * rename init_fxc as init_xc_kernel * update grad and lapl * delete useless matrix transformer * update comments * rename: lr_init_xc_kernel * add const before std::set * rename variables in PotHxcLR * rename gdr as gradrho * rename spinsize as n_component * use something else to replace the for-loop * [pre-commit.ci lite] apply automatic fixes * flatten the map * use std::any_of * rename new/delete_p2 as new/delete_pointer_dim2 * a usage of std::inner_product * rename 2order_nested_ptr * add annotation and minor changes Co-authored-by: kirk0830 <[email protected]> * redesign KernelXC: constructor sets all the members fix compile fix a initialize bug * rename and update doc --------- Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Co-authored-by: kirk0830 <[email protected]>
See the documentation of parameter
init_fxc.