Skip to content

Conversation

@maki49
Copy link
Collaborator

@maki49 maki49 commented Nov 1, 2024

See the documentation of parameter init_fxc.

@maki49 maki49 force-pushed the init_fxc branch 2 times, most recently from 21a1149 to 8bbbb2f Compare November 1, 2024 20:45
@mohanchen mohanchen added the Features Needed The features are indeed needed, and developers should have sophisticated knowledge label Nov 2, 2024
@maki49 maki49 force-pushed the init_fxc branch 2 times, most recently from 32d0022 to db47c6e Compare November 5, 2024 07:41
@maki49 maki49 force-pushed the init_fxc branch 4 times, most recently from 2e38a44 to dcee5b3 Compare November 5, 2024 12:53
@maki49 maki49 force-pushed the init_fxc branch 7 times, most recently from 7188343 to 97f2591 Compare November 6, 2024 19:19
Copy link
Collaborator

@kirk0830 kirk0830 left a 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.

  1. file structure is not organized well. In module_lr there is a file named esolver_lrtd_lcao, and all other files whose names startwith esolver are in folder module_esolver. I cannot tell immediately which is a better way but at least the inconsistency is obvious. In module_lr there are folders named operator_casida, potentials, and utils. However, in folder operator_casida, there is a file named operator_lr_hxc.cpp, it appears like there is a folder named fruit_apple and in it there is a file named plant. There are also files whose names contain lr, others contain lrtd, so I am confused what is the relationship between the lr and lrtd. I also note there is a a file named lr_util.h and then another lr_util.hpp, this is quite confusing, because .h usually indicates header file of C, and .hpp for C++, especially when they appear at the same time. Then there are files named lr_util and then lr_util_xc, there seems to be strong naming overlap: module_lr/utils/lr_util.*.
  2. There is inconsistency in the newly added keyword lr_init_xc_kernel. The linear-response is abbreviated as lr, initialize as init, exchange-correlation as xc but no abbr for kernel.
  3. 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.
  4. The instantiation of class PotHxcLR has explicit dependency on rank number, this design is not clearly explained.
  5. 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.
  6. 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.

@mohanchen
Copy link
Collaborator

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.

  1. file structure is not organized well. In module_lr there is a file named esolver_lrtd_lcao, and all other files whose names startwith esolver are in folder module_esolver. I cannot tell immediately which is a better way but at least the inconsistency is obvious. In module_lr there are folders named operator_casida, potentials, and utils. However, in folder operator_casida, there is a file named operator_lr_hxc.cpp, it appears like there is a folder named fruit_apple and in it there is a file named plant. There are also files whose names contain lr, others contain lrtd, so I am confused what is the relationship between the lr and lrtd. I also note there is a a file named lr_util.h and then another lr_util.hpp, this is quite confusing, because .h usually indicates header file of C, and .hpp for C++, especially when they appear at the same time. Then there are files named lr_util and then lr_util_xc, there seems to be strong naming overlap: module_lr/utils/lr_util.*.
  2. There is inconsistency in the newly added keyword lr_init_xc_kernel. The linear-response is abbreviated as lr, initialize as init, exchange-correlation as xc but no abbr for kernel.
  3. 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.
  4. The instantiation of class PotHxcLR has explicit dependency on rank number, this design is not clearly explained.
  5. 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.
  6. 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.

@maki49
Copy link
Collaborator Author

maki49 commented Nov 20, 2024

@kirk0830 Thanks for spending time to review. For point 5, the public members are all const references being used like getters, as what PARAM.inp does. Other points is something to be changed or discussed in another PR.

@mohanchen I have already changed all the things related to the feature implemented in this PR following his suggestion.

@mohanchen mohanchen merged commit 368495d into deepmodeling:develop Nov 21, 2024
14 checks passed
@maki49 maki49 deleted the init_fxc branch November 28, 2024 03:15
Fisherd99 pushed a commit to Fisherd99/abacus-BSE that referenced this pull request Mar 31, 2025
… 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]>
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