Refactor the relax module#7456
Conversation
主要修改： 1. 移除了 relax_driver.h 中未使用的 Ions_Move_BFGS2 bfgs_trad 成员变量及其头文件包含 2. 将 relax_driver.cpp 中 while 循环内的逻辑拆分为多个独立函数： - init_relax(): 初始化弛豫方法 - iter_info(): 打印迭代信息 - esolve(): 电子结构计算 - relax_step(): 执行单次弛豫步骤 - stru_out(): 输出结构文件 - json_out(): 输出 JSON 结果 - stop_cond(): 检查停止条件 - final_out(): 最终输出 3. 添加了成员变量用于缓存计算结果：force_, stress_, force_step, stress_step 4. 优化了 final_out() 函数，将重复的条件判断合并为早期返回 5. 所有成员变量和成员函数调用都添加了 this-> 前缀，提高代码可读性
mohanchen
force-pushed
the
20260608-relax
branch
from
5bb4525 to
110cd92
Compare
| @@ -87,87 +43,73 @@ void Ions_Move_CG::start(UnitCell &ucell, const ModuleBase::matrix &force, const | |||
| static double steplength = 0.0; | |||
| static double fmax = 0.0; | |||
| static int nbrent = 0; | |||
There was a problem hiding this comment.
The CG line-search state is still stored as function-local static variables (sd, trial, ncggrad, fa/fb/fc, xa/xb/xc, etc.).
This keeps the state shared across all instances of Ions_Move_CG. It is not a new behavior introduced by this PR, but since this PR is refactoring the relax module and moving other data into class-owned vectors, it would be better to move these variables into class members as well.
That would make the CG optimizer state explicit, easier to reset in allocate() / initialization, and safer if multiple optimizer objects are ever created in the same process.
| static bool sd = false; | ||
|
|
||
| // a cubic interpolation is used to make the third point, | ||
| // when sa = trial = false, we use Brent to get the | ||
| // minimum point in this direction. | ||
| static bool trial = false; | ||
|
|
||
| // ncggrad is a parameter to control the cg method, | ||
| // every ten cg direction, we change the direction back to | ||
| // the steepest descent method | ||
| static bool trial = false; | ||
| static int ncggrad = 0; | ||
|
|
||
| static double fa = 0.0; | ||
| static double fb = 0.0; | ||
| static double fc = 0.0; | ||
|
|
||
| static double xa = 0.0; | ||
| static double xb = 0.0; | ||
| static double xc = 0.0; | ||
|
|
||
| static double xpt = 0.0; | ||
| static double steplength = 0.0; | ||
| static double fmax = 0.0; | ||
|
|
||
| static int nbrent = 0; |
There was a problem hiding this comment.
This shares the same problem as the static local variables in Ions_Move_CG.
| if (PARAM.inp.cal_force) | ||
| { | ||
| p_esolver->cal_force(ucell, this->force_); | ||
| } | ||
|
|
||
| // calculate and gather all parts of total ionic forces | ||
| if (inp.cal_force) | ||
| { | ||
| p_esolver->cal_force(ucell, force); | ||
| } | ||
| else | ||
| { | ||
| // do nothing | ||
| } | ||
| if (PARAM.inp.cal_stress) | ||
| { | ||
| p_esolver->cal_stress(ucell, this->stress_); | ||
| } |
There was a problem hiding this comment.
Relax_Driver::relax_driver() receives const Input_para& inp, but some helper functions still read from the global PARAM.inp, for example esolve() checks PARAM.inp.cal_force and PARAM.inp.cal_stress.
Could we pass inp into esolve() and use inp.cal_force / inp.cal_stress there?
Using both an explicit input parameter and the global PARAM.inp in the same driver makes the data source less clear. Keeping the helper functions driven by the passed inp would make the refactor cleaner and reduce hidden global dependencies.
| void Ions_Move_Methods::cal_movement(const int &istep, | ||
| const int &force_step, | ||
| const ModuleBase::matrix &f, | ||
| const double &etot, | ||
| UnitCell &ucell) | ||
| UnitCell &ucell, | ||
| std::ofstream& ofs) | ||
| { | ||
| ModuleBase::TITLE("Ions_Move_Methods", "init"); | ||
| // Ions_Move_Basic::istep = istep; | ||
| Ions_Move_Basic::istep = force_step; | ||
| if (Ions_Move_Basic::relax_method[0] == "bfgs"&&Ions_Move_Basic::relax_method[1] != "1") |
There was a problem hiding this comment.
Ions_Move_Methods::allocate() now receives relax_method_0 and relax_method_1, but cal_movement() still dispatches based on the global Ions_Move_Basic::relax_method.
This makes the interface look instance-based during allocation, while the runtime behavior is still controlled by global state. It could also become inconsistent if Ions_Move_Basic::relax_method is changed after allocation, for example in the cg_bfgs path.
Could Ions_Move_Methods store the selected method as member variables during allocate() and use those members in cal_movement()?
2 participants
Refactor the relax module