Implementation of O1NumHess algorithm#1381
Implementation of O1NumHess algorithm#1381lmseidler wants to merge 56 commits intogrimme-lab:mainfrom
Conversation
63b54e9 to
3ea05ee
Compare
|
unfortunately under files changed it now also lists all the files modified by the rebased commits, for review mostly the o1numhess.f90, type/calculator.f90, test/unit/test_hessian.f90 files would be important edit: we should somehow configure pre-commit hooks such that it doesn't happen anymore that someone pushes unsigned commits accidentally to avoid the whole rebasing/force-pushing stuff |
|
This PR is really hard to check. Please, rebase all your changes to the current main branch. You can easily do it with the following sequence of commands: git clone git@github.com:grimme-lab/xtb.git xtb.1381
cd xtb.1381
git remote add lmseidler git@github.com:lmseidler/xtb.git
git fetch lmseidler
git checkout lmseidler/o1numhess
git rebase origin/main
# I use kdiff3 for resolving conflicts
git mergetool --tool=kdiff3
# solve conflicts in src/extern/orca.f90; take 3rd variant of code with inquire.
git rebase --continue # continue after stopping
# I created a new branch fixtran/1381 below; you should check it before overwriting lmseidler/o1numhess.
# Hope, you will not need in `git reflog`
git checkout -b fixtran/1381
git push lmseidler |
|
done |
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
Signed-off-by: lmseidler <seidler118@gmail.com>
chselz
left a comment
There was a problem hiding this comment.
Some minor things i would correct
however i did not look into implementation details of o1numhess itself, i.e. checking if theory itself was implemented correctly
Signed-off-by: lmseidler <seidler118@gmail.com>
chselz
left a comment
There was a problem hiding this comment.
I think it should be fine now
Before merging probably someone else should also approve as this is quite a big PR
Signed-off-by: Leopold Seidler <seidler118@gmail.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements support for a new Hessian calculation method using the ODLR (O1NumHess) approximation based on Wang et al. (2025). The implementation adds a new command-line option --o1nh, introduces comprehensive utility functions in a new o1numhess module, adds parameter data for covalent and van der Waals radii, updates the workflow to disable IR/Raman intensity calculations when using this method, and includes an extensive test suite with fixture support.
Changes:
- Implements ODLR Hessian calculation algorithm with optimized displacement direction generation and low-rank corrections
- Adds test infrastructure including fixture system and comprehensive unit tests for algorithm components
- Integrates new method into existing workflow with compatibility checks and appropriate warnings
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/o1numhess.f90 | New module implementing O1NumHess algorithm utilities including neighbor lists, displacement direction generation, conjugate gradient solver, and modified Swart model Hessian |
| src/type/calculator.f90 | Added odlrhessian method to TCalculator class with gradient derivative calculation helpers |
| src/hessian.F90 | Integrated ODLR Hessian into numhess workflow with conditional execution |
| src/param/covalentrad.f90 | New module providing covalent radii data (Pyykko and Atsumi) |
| src/param/uffvdwrad.f90 | New module providing UFF van der Waals radii data |
| src/setparam.f90 | Added o1numhess logical flag to settings |
| src/prog/main.F90 | Added command-line parsing for --o1nh option and compatibility validation |
| src/main/property.F90 | Conditionally disabled IR/Raman output when using ODLR Hessian |
| src/xhelp.f90 | Added help text and citation for O1NumHess |
| man/xtb.1.adoc | Added manual entry for --o1nh option |
| test/unit/test_hessian.f90 | Comprehensive test suite including fixture support and algorithm component tests |
| test/unit/meson.build, test/unit/CMakeLists.txt | Updated build systems to set TEST_FIXTURES_DIR environment variable |
| src/param/meson.build, src/param/CMakeLists.txt | Added new parameter modules to build systems |
| src/meson.build, src/CMakeLists.txt | Added o1numhess.f90 to build systems |
| README.md | Added contributor and O1NumHess reference |
| assets/references.bib | Added bibtex entry for Wang et al. paper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Leopold Seidler <seidler118@gmail.com>
Signed-off-by: Leopold Seidler <seidler118@gmail.com>
Signed-off-by: Leopold Seidler <seidler118@gmail.com>
| & 2.01_wp, 1.86_wp, 1.75_wp, 1.69_wp, 1.70_wp, 1.71_wp, 1.72_wp, 1.66_wp, 1.66_wp, 1.68_wp, 1.68_wp, & | ||
| & 1.65_wp, 1.67_wp, 1.73_wp, 1.76_wp, 1.61_wp] / autoaa | ||
|
|
||
| ! NOTE: these should later be moved to mctc-lib |
There was a problem hiding this comment.
Can't we just use the mctc-lib radii directly and scale them back to their original value?
| ! | ||
| ! You should have received a copy of the GNU Lesser General Public License | ||
| ! along with xtb. If not, see <https://www.gnu.org/licenses/>. | ||
| !> UFF vdw radii - could be replaced with any other vdw radii i guess |
There was a problem hiding this comment.
Should we put those into mctc-lib or just use our vdw radii?
| end if | ||
| call env%warning("O1NumHess does not support calculation of IR or Raman activities", source) | ||
| if (set%step_hess >= 0.0005_wp) then | ||
| call env%warning("Step size for O1NumHess can be chosen to be much smaller (recommended: 1.0e-6)", source) |
There was a problem hiding this comment.
1e-6 seems very small. Is our SCF convergence even tight enough for this?
| !> Final error after all steps | ||
| real(wp), intent(out) :: final_err | ||
|
|
||
| real(wp), parameter :: dmax = 1.0_wp, eps = 1.0e-8_wp, eps2 = 1.0e-15_wp, imagthr = 1.0e-8_wp |
There was a problem hiding this comment.
should those really be hard-coded constants? Can we make them dependent on the epsilon for a floating point operation?
|
|
||
| end subroutine odlrhessian | ||
|
|
||
| subroutine get_gradient_derivs(self, env, step, ndispl0, ndispl_final, displdir, mol0, chk0, g0, doublesided, g) |
There was a problem hiding this comment.
Can we use this consistently also in the normal hessian?
| i2 = 3 * i | ||
| j1 = 3 * j - 2 | ||
| j2 = 3 * j | ||
| hess_local(i1:i2, i1:i2) = hess_local(i1:i2, i1:i2) + Hint * outer6(1:3, 1:3) |
There was a problem hiding this comment.
Please add some explanation on what is happening here
| integer :: length, stat | ||
|
|
||
| ! Get variable; status 0 means it exists | ||
| call get_environment_variable("TEST_FIXTURES_DIR", base_dir, length, stat) |
There was a problem hiding this comment.
This is just for the reference neighbor list and distance matrix? Can't we add to the test directory a data folder where they are always stored. Then we don't need to do any path resolution
|
@lmseidler Hey, I have a working implementation of O1HessFast since few months for xtb. I didn't have time to open pull request. But maybe you could benefit from using my implementation make it work or copy/paste part of it? The core difference is that there that this version is heavily optimized for the parallel calculations, to make it extremely fast. The code is under https://github.com/DanielWicz/xtb/tree/calcnested It starts with commit: DanielWicz@0a8b51c There are few additional steps added (additional sampling of degrees of freedom) to remove imaginary frequencies on top of the method. Upon compilation it works like this: xtb structure.xyz --ohess (or --hess) --onehessfast Or if anyone has some ideas, I can e.g. try to merge with @lmseidler. I just need to know how functional is your current code. |
|
Hi, the current version is basically usable but not very polished, which I didn't get to yet. What would definitely interesting if you managed to get an efficiently parallelized version to get the displacement vectors? This is currently kind of a bottleneck in the parallelization. I will check out your version and compare, also for other things, then maybe we can merge some stuff. |
This pull request introduces support for a new Hessian calculation method using the ODLR (O1NumHess) approximation. The main changes include adding the new
--o1nhcommand-line option, implementing the ODLR Hessian algorithm, and updating the workflow and output to accommodate this new method. The implementation ensures that the ODLR Hessian is only used when appropriate and disables IR/Raman intensity calculations when this method is selected.Key changes:
ODLR Hessian Implementation
odlrhessiansubroutine to theTCalculatorclass, implementing the ODLR (O1NumHess) Hessian calculation based on Wang et al. (https://doi.org/10.1021/acs.jctc.5c01354).numhess, callingodlrhessianwhen the new method is selected.User Interface and Workflow
--o1nhcommand-line option to enable the ODLR Hessian calculation, and documented it in the manual. [1] [2]o1numhesslogical flag to the settings structure, and ensured it is properly parsed and propagated throughout the codebase.Output and Feature Adjustments
Codebase and Build System
o1numhess.f90to the build system.Miscellaneous