deepmodeling / abacus-develop

An electronic structure package based on either plane wave basis or numerical atomic orbitals.
http://abacus.ustc.edu.cn
GNU Lesser General Public License v3.0
174 stars 136 forks source link

Function allocate_psi_init and initialize_psi should be remove from esolver_ks_pw.cpp #4401

Closed Qianruipku closed 5 months ago

Qianruipku commented 5 months ago

Describe the Code Quality Issue

Esolver_KS_PW defined too much functions and they should be moved to other places.

Additional Context

No response

Task list for Issue attackers (only for developers)

kirk0830 commented 5 months ago

Sorry but probably I don't think so. The initialization of a psi instance should be the event occurs in ESolver_KS_PW workflow (not ESolver_KS because it is the initialization of PW wavefunction), it is the reason why these two functions are there. Similarly, for lcao, the most suitable choice still would be in ESolver_KS_LCAO. The opinion above is based on the fact that the instantiation/allocation/assignment in ABACUS still be ridiculously separated into pieces and always hide their calls somewhere, and I clearly remember in function Init_GlobalC there is one quite funny thing: one must first call function to allocate psi, then call some functions to init other things which seem to be irrelevant at all, and at the end, assign values to psi. If this order is not strictly obeyed, there will be undefined behavior! Thus, if be consistent with your opinion, I think Init_GlobalC should also/first be removed.

So if for a fully good design, there are two more reasonable things to do:

  1. add one more constructor for psi, then directly do instantiation-allocation-assignment together, so that Init_GlobalC will lose part of its functionality. But if really do like this, then psi will lose its identity as a purely cross-device data container, because the constructor gives its somewhat physical meaning (if I understand the design of psi correctly @denghuilu), this is definitely a pollution, but there are plenty of classes have been severely polluted and far from good design C++ code, resulting in the waste of time on endless refactor for all developers. So at the end it comes to the conclusion again "adding one interface between data container and physical meaning quantity", yes that what those functions do. So the choice will be more complicated and if one has noticed there are Tensor instead of psi is used in HSolver, it implies Tensor will be the data container and psi seems "can have some physical meaning". Then the memory copy (plus other memory operations) should be the work of Tensor rather than psi. I hope the constructor caused pollution will not last for too long time.
  2. write/modify the projection of numerical atomic orbital onto pw basis to be a static function and returns one psi, during calculation it is dynamically called, then LCAO_IN_PW, init_wfc nao and to_wannier90_lcao_in_pw three functionalities can reduce its memory cost.