Skip to content
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

Implement a new BFGS optimizer, used for geometry relaxation #5467

Merged
merged 31 commits into from
Dec 1, 2024

Conversation

19hello
Copy link
Collaborator

@19hello 19hello commented Nov 12, 2024

Reminder

  • Have you linked an issue with this pull request?
  • Have you added adequate unit tests and/or case tests for your pull request?
  • Have you noticed possible changes of behavior below or in the linked issue?
  • Have you explained the changes of codes in core modules of ESolver, HSolver, ElecState, Hamilt, Operator or Psi? (ignore if not applicable)

Linked Issue

Fix #...

Unit Tests and/or Case Tests for my changes

  • A unit test is added for each new feature or bug fix.

What's changed?

  • Example: My changes might affect the performance of the application under certain conditions, and I have tested the impact on various scenarios...

Any changes of core modules? (ignore if not applicable)

  • Example: I have added a new virtual function in the esolver base class in order to ...

@dyzheng
Copy link
Collaborator

dyzheng commented Nov 13, 2024

Please rewrite the title of this PR, where is this method from?

@dyzheng
Copy link
Collaborator

dyzheng commented Nov 13, 2024

Please raise a issue to describe this method you have implemented, and show some results to prove your code.

@QuantumMisaka
Copy link
Collaborator

@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 Show resolved Hide resolved
SiH2-3et-relax/Si.ccECP.upf Outdated Show resolved Hide resolved
SiH2-3et-relax/abacus.out Outdated Show resolved Hide resolved
SiH2-3et-relax/log Outdated Show resolved Hide resolved
tests/integrate/109_PW_CR/log1 Outdated Show resolved Hide resolved
@19hello 19hello closed this Nov 17, 2024
@19hello 19hello reopened this Nov 17, 2024
@QuantumMisaka
Copy link
Collaborator

I'll have test with #3119 and my system

@QuantumMisaka
Copy link
Collaborator

@19hello After you finish adding your BFGS method, please consider the way user know and use it

@mohanchen mohanchen changed the title Mybfgs Implement a new BFGS optimizer, used for geometry relaxation Nov 19, 2024
source/module_relax/CMakeLists.txt Outdated Show resolved Hide resolved
source/module_relax/CMakeLists.txt Outdated Show resolved Hide resolved
source/module_relax/relax_driver.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_driver.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_driver.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_new/bfgs.h Outdated Show resolved Hide resolved
source/module_relax/relax_new/test/bfgs_test.cpp Outdated Show resolved Hide resolved
@mohanchen mohanchen added Features Needed The features are indeed needed, and developers should have sophisticated knowledge GeometryRelaxation Issues related to geometry relaxation Refactor Refactor ABACUS codes labels Nov 19, 2024
@kirk0830
Copy link
Collaborator

kirk0830 commented Nov 20, 2024

I will review this PR carefully (mainly focus on the code structure) once all functionalities are implemented correctly.

@QuantumMisaka
Copy link
Collaborator

@19hello Now the stdout in BFGS:

 STEP OF ION RELAXATION : 1
 -------------------------------------------
 START CHARGE      : atomic
 DONE(12.0107    SEC) : INIT SCF
 ITER      TMAG       AMAG        ETOT/eV          EDIFF/eV         DRHO     TIME/s
 GE1      1.00e+00   1.05e+00  -1.05828247e+05   0.00000000e+00   2.0197e-01  27.87
 GE2      1.00e+00   1.03e+00  -1.05974176e+05  -1.45929058e+02   6.3496e-02  26.71
 GE3      1.00e+00   1.05e+00  -1.05975481e+05  -1.30541467e+00   4.0279e-02  26.62
 GE4      1.00e+00   1.03e+00  -1.05975750e+05  -2.69111778e-01   1.6323e-02  26.54
 GE5      1.00e+00   1.04e+00  -1.05975788e+05  -3.79099791e-02   2.5708e-03  26.61
 GE6      1.00e+00   1.04e+00  -1.05975801e+05  -1.32110432e-02   1.6555e-03  26.52
 GE7      1.00e+00   1.04e+00  -1.05975807e+05  -5.65212981e-03   9.8648e-04  26.54
 GE8      1.00e+00   1.04e+00  -1.05975810e+05  -2.82251525e-03   5.1189e-04  26.47
 GE9      1.00e+00   1.04e+00  -1.05975812e+05  -2.36488872e-03   2.8607e-04  26.58
 GE10     1.00e+00   1.04e+00  -1.05975813e+05  -1.23262177e-03   1.6751e-04  26.52
 GE11     1.00e+00   1.04e+00  -1.05975815e+05  -1.12359819e-03   1.2231e-04  26.49
 GE12     1.00e+00   1.04e+00  -1.05975815e+05  -8.95499827e-04   8.2590e-05  26.47
 GE13     1.00e+00   1.04e+00  -1.05975816e+05  -6.48718030e-04   5.4154e-05  26.49
 GE14     1.00e+00   1.04e+00  -1.05975817e+05  -4.14907776e-04   3.2547e-05  26.61
 GE15     1.00e+00   1.04e+00  -1.05975817e+05  -1.32733026e-04   1.7303e-05  26.47
 GE16     1.00e+00   1.04e+00  -1.05975817e+05  -8.18803778e-05   1.1003e-05  26.48
 GE17     1.00e+00   1.04e+00  -1.05975817e+05  -3.53268862e-05   5.9376e-06  26.52
 GE18     1.00e+00   1.04e+00  -1.05975817e+05  -1.76772219e-05   3.7520e-06  26.55
 GE19     1.00e+00   1.04e+00  -1.05975817e+05  -1.47325444e-05   2.2837e-06  26.47
 GE20     1.00e+00   1.04e+00  -1.05975817e+05  -9.94843809e-06   1.3589e-06  26.47
 GE21     1.00e+00   1.04e+00  -1.05975817e+05  -1.20947498e-05   7.7700e-07  26.41
----------------------------------------------------------------
 TOTAL-STRESS (KBAR)                                            
----------------------------------------------------------------
        20.4609576207        -0.3745619029         0.2831022240 
        -0.3745619029        18.6297901586         1.0074961155 
         0.2831022240         1.0074961155        20.2788212288 
----------------------------------------------------------------
 TOTAL-PRESSURE: 19.789856 KBAR

-------------------------------------------
 STEP OF ION RELAXATION : 2
 -------------------------------------------
 DONE(603.992246 SEC) : INIT SCF

The LARGEST GRAD information is lost, please fix it

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.

thanks for your contribution, I have roughly reviewed your PR. There are mainly two points that you should consider additionally:

  1. 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
  2. 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:

  1. 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.
  2. 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.
  3. Unittest and integrated tests are absent.

source/module_io/read_input_item_relax.cpp Show resolved Hide resolved
@QuantumMisaka
Copy link
Collaborator

@19hello Thanks for you contribution!
Please notices:

  1. You need an issue to let developers and users know your development target and background
  2. Your function should have likely print-out format in stdout and running*.log to allow smoothly usage

Besides, there are some other optimizer for geometry relaxation in ASE, including the most recommended BFGSLineSearch : https://wiki.fysik.dtu.dk/ase/ase/optimize.html#bfgslinesearch, optimizer base on MD (MDMin and FIRE), and optimizer from scipy. There are also many types of BFGS, so you need to explain what your BFGS is.

Furthermore, if your BFGS coding structure is beauty, it will benefit your next optimizer implementation. I suggest starting from BFGSLineSearch

@QuantumMisaka
Copy link
Collaborator

I'll have test with #3119 and my system

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 LARGEST GRAD information is lost in the stdout, and I do not know if the convergence judgement is also lost in your algorithm, please check them,

@19hello
Copy link
Collaborator Author

19hello commented Nov 21, 2024

I'll have test with #3119 and my system

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 LARGEST GRAD information is lost in the stdout, and I do not know if the convergence judgement is also lost in your algorithm, please check them,

Thank you for your testing. I will try to fix the problems you mentioned in next PR

@QuantumMisaka
Copy link
Collaborator

I'll have test with #3119 and my system

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 LARGEST GRAD information is lost in the stdout, and I do not know if the convergence judgement is also lost in your algorithm, please check them,

Thank you for your testing. I will try to fix the problems you mentioned in next PR

I consider the LARGEST GRAD information in stdout should be fixed in this PR

@19hello
Copy link
Collaborator Author

19hello commented Nov 21, 2024

I'll have test with #3119 and my system

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 LARGEST GRAD information is lost in the stdout, and I do not know if the convergence judgement is also lost in your algorithm, please check them,

Thank you for your testing. I will try to fix the problems you mentioned in next PR

I consider the LARGEST GRAD information in stdout should be fixed in this PR

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.

@QuantumMisaka
Copy link
Collaborator

I'll have test with #3119 and my system

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 LARGEST GRAD information is lost in the stdout, and I do not know if the convergence judgement is also lost in your algorithm, please check them,

Thank you for your testing. I will try to fix the problems you mentioned in next PR

I consider the LARGEST GRAD information in stdout should be fixed in this PR

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 force_thr_ev parameter ?

@19hello
Copy link
Collaborator Author

19hello commented Nov 21, 2024

I'll have test with #3119 and my system

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 LARGEST GRAD information is lost in the stdout, and I do not know if the convergence judgement is also lost in your algorithm, please check them,

Thank you for your testing. I will try to fix the problems you mentioned in next PR

I consider the LARGEST GRAD information in stdout should be fixed in this PR

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 force_thr_ev parameter ?

I forgot it. I will change the convergence condition in next PR.

@QuantumMisaka
Copy link
Collaborator

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 force_ev_thr been used and more optimization method added-in

source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
source/module_relax/relax_old/bfgs.cpp Outdated Show resolved Hide resolved
@mohanchen mohanchen merged commit 028afb0 into deepmodeling:develop Dec 1, 2024
14 checks passed
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 GeometryRelaxation Issues related to geometry relaxation Refactor Refactor ABACUS codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants