duartegroup / autodE

automated reaction profile generation
https://duartegroup.github.io/autodE/
MIT License
173 stars 52 forks source link

Improved CRFO and PRFO optimiser #344

Closed shoubhikraj closed 4 months ago

shoubhikraj commented 5 months ago

(1) Use Lagrangian multiplier weights to identify correct constraint modes in CRFO (2) Update the molecular hessian instead of the lagrangian hessian for CRFO (3) Optional mode-following by overlap checking in PRFO


Checklist

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 99.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.39%. Comparing base (cb45e53) to head (38de918).

Files Patch % Lines
autode/opt/optimisers/crfo.py 93.75% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## v1.4.4 #344 +/- ## ========================================== - Coverage 97.41% 97.39% -0.03% ========================================== Files 202 202 Lines 23210 23276 +66 ========================================== + Hits 22611 22670 +59 - Misses 599 606 +7 ``` | [Flag](https://app.codecov.io/gh/duartegroup/autodE/pull/344/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=duartegroup) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/duartegroup/autodE/pull/344/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=duartegroup) | `97.39% <99.42%> (-0.03%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=duartegroup#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

shoubhikraj commented 4 months ago

@t-young31 When you have time, could you please check this again?

shoubhikraj commented 4 months ago

@t-young31 I have been running many more tests on this, and it seems that there are still many problems - and the optimiser struggles when the geometry has other negative eigenvalues (so for example taking a SN2 TS and constraining a C-H bond that has no link to the imaginary mode) - and taking the eigenvectors with highest lagrange multiplier weight does not solve the problem in all cases (as the original paper suggested). There is also no open source code that I could find which uses this method on geometry optimisation.

What do you think about replacing the lagrange multiplier method entirely with a simple force constant penalty (k x^2) for the constraints as in xTB? It would be easier to implement and also make the optimiser simpler as there is no extra mode following involved.

t-young31 commented 4 months ago

I'm not hugely surprised that all hell breaks loose with non-constraint imaginary modes. What happens if you force positive definiteness?

shoubhikraj commented 4 months ago

@t-young31 Thank you! that actually solved the problem. I used the RFO shift parameter to force the hessian to be positive definite - and it can handle even very complicated cases where there are other unrelated negative eigenvalues in the molecule.

shoubhikraj commented 4 months ago

@t-young31 Happy for me merge this?

t-young31 commented 4 months ago

just needs a changelog update I think 👍🏼