Implement a new BFGS optimizer, used for geometry relaxation #5467
Conversation
|
Please rewrite the title of this PR, where is this method from? |
|
Please raise a issue to describe this method you have implemented, and show some results to prove your code. |
|
@19hello Apart from the method description, from the users' side we need test results from your BFGS method, and compare test for old BFGS and ASE BFGS |
SiH2-3et-relax/H.ccECP.upf
Outdated
There was a problem hiding this comment.
do not add new file, this file is too large
There was a problem hiding this comment.
@19hello
for SiH2 example, you can attach it as zip file under the corresponding issue, but not in source-code
There was a problem hiding this comment.
I've delete SiH2-3et-relax
There was a problem hiding this comment.
@19hello After you resolve this comment, please mark them as resolved
SiH2-3et-relax/log
Outdated
There was a problem hiding this comment.
we don't need this file
|
I'll have test with #3119 and my system |
|
@19hello After you finish adding your BFGS method, please consider the way user know and use it
|
mohanchen
changed the title
source/module_relax/CMakeLists.txt
Outdated
| @@ -6,6 +6,7 @@ add_library( | |||
|
|
|||
| relax_new/relax.cpp | |||
| relax_new/line_search.cpp | |||
| relax_new/bfgs.cpp | |||
|
|
|||
There was a problem hiding this comment.
one extra line found here, please delete it
source/module_relax/CMakeLists.txt
Outdated
| @@ -17,6 +18,7 @@ add_library( | |||
| relax_old/lattice_change_basic.cpp | |||
| relax_old/lattice_change_cg.cpp | |||
| relax_old/lattice_change_methods.cpp | |||
|
|
|||
source/module_relax/relax_driver.cpp
Outdated
| { | ||
| if (!PARAM.inp.relax_new) | ||
| if(PARAM.inp.relax_method == "bfgs_trad") |
There was a problem hiding this comment.
some logic here have problems, please think carefully the function of 'relax_new' parameter
There was a problem hiding this comment.
@sunliang98 please help Feiyang check the codes
There was a problem hiding this comment.
@YuLiu98 please help checking the logic here
source/module_relax/relax_driver.cpp
Outdated
| @@ -77,7 +82,12 @@ void Relax_Driver::relax_driver(ModuleESolver::ESolver* p_esolver) | |||
|
|
|||
| if (PARAM.inp.calculation == "relax" || PARAM.inp.calculation == "cell-relax") | |||
| { | |||
| if (PARAM.inp.relax_new) | |||
| if(PARAM.inp.relax_method == "bfgs_trad") | |||
There was a problem hiding this comment.
same logic problem found here
source/module_relax/relax_driver.cpp
Outdated
| { | ||
| stop=bfgs_trad.relax_step(force,GlobalC::ucell); | ||
| } | ||
|
|
There was a problem hiding this comment.
one extra line found, please delete it
source/module_relax/relax_new/bfgs.h
Outdated
| { | ||
| public: | ||
|
|
||
| double alpha;//initialize H,diagonal element is alpha |
There was a problem hiding this comment.
use doxygen format for notes
There was a problem hiding this comment.
you need to learn how to add tests.
|
@19hello Please remove redundant information in stdout and add standard stdout information Standard |
mohanchen
added
Features Needed
|
I will review this PR carefully (mainly focus on the code structure) once all functionalities are implemented correctly. |
|
@19hello Now the stdout in BFGS: The LARGEST GRAD information is lost, please fix it |
There was a problem hiding this comment.
thanks for your contribution, I have roughly reviewed your PR. There are mainly two points that you should consider additionally:
- explain about the term "bfgs_trad", in this PR and also update our markdown document in /docs/advanced/input_files/input-main.md. Because not only us, all users should have a way to know the functionality you implemented
- there are really few annotation both in you header file and source file. Please use doxygen format to add some anntations.
There are also other aspects in which this BFGS implementation can be significantly improved, but you are not required to do these in this PR. There will be a issue records all possible improvements. Some of them are:
- the code structure and interface design. The former is really a time-consuming task, before having a very clear idea, I will not talk too much here. The latter is more specific: because BFGS is a general optimization algorithm, once you implement one with higher efficiency, all other developers can benefit from you code as long as the interface is general enough.
- The linear algebra (linalg) operation (op). I notice you use std::vector of std::vector to represent a matrix, and you write some linalg ops on your own. This will not significantly affect performance in small systems, but will be bottle neck when the system is quite large.
- Unittest and integrated tests are absent.
|
@19hello Thanks for you contribution!
Besides, there are some other optimizer for geometry relaxation in ASE, including the most recommended Furthermore, if your BFGS coding structure is beauty, it will benefit your next optimizer implementation. I suggest starting from BFGSLineSearch |
In my easy test, this BFGS somehow have worse relaxation performance, which lead to 50 ion step but not converge, but the original BFGS can make it done in 40 ion steps. But the |
Thank you for your testing. I will try to fix the problems you mentioned in next PR |
I consider the |
I just pushed a new PR. In this PR, I have add 'LARGEST GRAD ` information in stdout. I also changed the logic of obtaining atom pos and add Unittest and integrated tests. The previous convergence condition was that dpos less than 0.00001, so it can't converge within 50 steps. In the new PR, you can see the 'LARGEST GRAD ` information to judge if it is converged. Besides, I can't run #3119 on my computer because the files are so large. Would you please run my bfgs_trad again. |
Good, I'll test it, but is there any diffuculty to normalize the convergence condition to the traditional |
I forgot it. I will change the convergence condition in next PR. |
|
Optimization through this BFGS have been test on #3119 and showing much efficiency (reduce 50% of the number of optimization steps to 0.05 eV/A max force and show no sharp increase of LARGEST GRAD). Waiting for the |
|
|
||
| void BFGS::allocate(const int _size) // initialize H0、H、pos0、force0、force | ||
| { | ||
| alpha=70;//relax_scale_force |
There was a problem hiding this comment.
what does the alpha number mean?
| ucell.ionic_position_updated = true; | ||
| for(int i = 0; i < _force.nr; i++) | ||
| { | ||
|
|
| } | ||
|
|
||
|
|
||
| void BFGS::PrepareStep(std::vector<std::vector<double>>& force,std::vector<std::vector<double>>& pos,std::vector<std::vector<double>>& H,std::vector<double>& pos0,std::vector<double>& force0,std::vector<double>& steplength,std::vector<std::vector<double>>& dpos,UnitCell& ucell) |
There was a problem hiding this comment.
one variable for a line
| std::vector<double> omega(3*size); | ||
| std::vector<double> work(3*size*3*size); | ||
| int lwork=3*size*3*size; | ||
| int info; |
| force0 = this->ReshapeMToV(force); | ||
| } | ||
|
|
||
| void BFGS::Update(std::vector<double> pos, std::vector<double> force,std::vector<std::vector<double>>& H,UnitCell& ucell) |
There was a problem hiding this comment.
use &pos instead of pos, use &force instead of force
| a=w; | ||
| } | ||
| } | ||
| } |
| Ions_Move_Basic::converged = Ions_Move_Basic::largest_grad * ModuleBase::Ry_to_eV / 0.529177<PARAM.inp.force_thr_ev; | ||
| } | ||
|
|
||
| void BFGS::CalculateLargestGrad(ModuleBase::matrix _force,UnitCell& ucell) |
There was a problem hiding this comment.
use &_force instead of _force
|
|
||
| // matrix methods | ||
|
|
||
| std::vector<double> BFGS::ReshapeMToV(std::vector<std::vector<double>> matrix) |
There was a problem hiding this comment.
use &matrix instead of matrix
| return result; | ||
| } | ||
|
|
||
| std::vector<std::vector<double>> BFGS::MAddM(std::vector<std::vector<double>> a, std::vector<std::vector<double>> b) |
| return result; | ||
| } | ||
|
|
||
| std::vector<double> BFGS::VSubV(std::vector<double> a, std::vector<double> b) |
There was a problem hiding this comment.
this is strange, I don't think you need to implement the function by yourself
Reminder
Linked Issue
Fix #...
Unit Tests and/or Case Tests for my changes
What's changed?
Any changes of core modules? (ignore if not applicable)