Improvements of LZSH algorithm by JanosJiri · Pull Request #159 · PHOTOX/ABIN

JanosJiri · 2023-04-12T07:13:50Z

This commit slightly modifies the LZ code to fix issues we encountered for several molecules.

LZ is computed just for the neighbouring states, not all that are available. Previously, the implementation allowed for unphysical hops like S1 --> S4.
Warning is printed out when the sum of hopping probabilities to the two neighbouring states is larger than one. ABIN somehow deals with that but LZ formula is no longer valid and would need a 3-state generalization.
Warning is printed out when possible discontinuities are encountered. It is based on second derivatives. Testing on azobenzene and bilirubin showed it is able to warn the user when hops due to discontinuity happen.
Warning at the beginning of output if there are more than two states of the same multiplicity. In such cases, LZ works only when the states are well separated. This doesn't hold true for azobenzene, bilirubin, porphyrin and other molecules and LZ should not be used.
Check for dE>deltaE_lz is added at the beginning, otherwise it messes up with probabilities.

Btw, the simple velocity rescaling right be also a problem for LZSH but this was not modified.

Current implementation allows for hops like (05-->01), which is not compatible with the LZ formula that was derived for two-state problem. This commit changes it and evaluates it only for closest neighbours.

LZSH exhibits large problems when discontinuities in energies are encountered. Simple warning based on large change in second derivatives is added. It was tested on azobenzene and bilirubin and it was able to warn when suspicious hops occured. Also warning about too many states was added at the beginning of the output file.

This unfortunately means we can no longer test with gfortran-7.

danielhollas

Cool! These look like very important fixes. I mostly have just a couple of clarifying questions.
In general, don't be afraid to spell things out a bit more in the code comments to make things super-clear for anybody who might look at this code. Thanks!

(note, the tests are not running because of the old Ubuntu version, I am fixing that in #160

The only significant change is a new condition for discontinuities.

GHA: Update Ubuntu version to 20.04

danielhollas · 2023-04-18T20:55:40Z

@JanosJiri are you compiling locally with MPI? Looks like the TERAPI-LZ tests are failing.

JanosJiri · 2023-04-19T14:13:30Z

@danielhollas No, I didn't compile it with MPI. I had general issues with MPI here so I even did not try.

danielhollas · 2023-05-22T10:10:20Z

@JanosJiri anything I can help with to get this over the finish line?

JanosJiri · 2023-05-25T13:34:32Z

@JanosJiri anything I can help with to get this over the finish line?

@danielhollas I know the problem is in the TERAPI-LZ test but I was not able to compile ABIN with MPI here. So I don't see where exactly it failed. I just know it did not hop in the test suite. I will try to compile it again but I won't have time to do it soon.

danielhollas · 2023-05-25T14:49:09Z

Cool, thanks! In terms of compiling with MPI, I'd recommend using the dev_scripts/install_mpich.sh to compile your own MPICH version locally, and then rerunning configure with ./configure --mpi /path/to/mpi. This is what we use on Github CI so it should work.

danielhollas · 2023-06-26T12:54:53Z

@JanosJiri I've looked at the failing test TERAPI-LZ, and the issue is that in the original test the system hops from 3rd to 2nd state in the third timestep, but with the new code the output in abin.out says

 Three-point minimum (03->02) dE/a.u.   4.2773499999952946E-003 Probability:  0.95692859086662962
 ERROR: Change of curvature --> discontinuity in PES!
 Probability set to 0!

I am not sure if this is expected, since the PES seems fine to me at first glance. Here are the curves between S2 and S1 states

Zoomed out view that includes S0

(of course this is a synthetic test but the curves don't seem unrealistic to me)

JanosJiri · 2023-06-27T06:41:04Z

@JanosJiri I've looked at the failing test TERAPI-LZ, and the issue is that in the original test the system hops from 3rd to 2nd state in the third timestep, but with the new code the output in abin.out says
 Three-point minimum (03->02) dE/a.u.   4.2773499999952946E-003 Probability:  0.95692859086662962
 ERROR: Change of curvature --> discontinuity in PES!
 Probability set to 0!
I am not sure if this is expected, since the PES seems fine to me at first glance. Here are the curves between S2 and S1 states

Zoomed out view that includes S0

(of course this is a synthetic test but the curves don't seem unrealistic to me)

@danielhollas Thanks for looking at it and also sorry for taking such a long time with it. I believe the problem is in the test, not in the code. The 'smoothness' check I implemented compares the current second derivative of the energy difference with the previous one. However, since this hops on the third step of the algorithm, there is no previous second derivative to compare with. Or rather the 4th energy in the array is still zero generating an artificial second derivative. So I believe this can be solved by adding one more data point before the hopping step. All should be fine then. I will have a look but in a three weeks when I will be back from holidays.

danielhollas · 2023-06-27T12:23:51Z

No worries at all!

Ah, that is tricky indeed. Let's discuss when you get back. Enjoy the well deserved break!

Modification of TERAPI-LZ test to be compatible with the new modifications of the LZ algorithm. The problem was just three steps. Thus, the previous second derivative used in the discontinuity check was still set to 0 which caused the code to think there is a discontinuity. One more step was artificially added so that it has also the previous second derivative.

JanosJiri · 2023-07-14T12:55:42Z

@danielhollas I modified the test so that it has one more step before hopping. It worked on neon so hopefully it will work also here.

danielhollas · 2023-07-19T16:01:46Z

@JanosJiri Thanks for fixing the test!

I believe the problem is in the test, not in the code. The 'smoothness' check I implemented compares the current second derivative of the energy difference with the previous one. However, since this hops on the third step of the algorithm, there is no previous second derivative to compare with. Or rather the 4th energy in the array is still zero generating an artificial second derivative.

I am not sure I agree that the problem was in the test though (or at least I wouldn't phrase it that way). Before this PR it was perfectly legal to jump on the third step (but not in the first two steps). After this change, we're essentially disallowing any hopping before 4th steps. I guess that is okay, but I am not very happy that there is a misleading warning emitted if the system wants to jump in the third step (i.e. user might think there was discontinuity, even though in reality there wasn't).

@JanosJiri Let me know your thoughts. I am happy to merge this as is, and we can improve upon this later. 🚀

JanosJiri · 2023-07-27T09:51:19Z

@JanosJiri Thanks for fixing the test!

I believe the problem is in the test, not in the code. The 'smoothness' check I implemented compares the current second derivative of the energy difference with the previous one. However, since this hops on the third step of the algorithm, there is no previous second derivative to compare with. Or rather the 4th energy in the array is still zero generating an artificial second derivative.

I am not sure I agree that the problem was in the test though (or at least I wouldn't phrase it that way). Before this PR it was perfectly legal to jump on the third step (but not in the first two steps). After this change, we're essentially disallowing any hopping before 4th steps. I guess that is okay, but I am not very happy that there is a misleading warning emitted if the system wants to jump in the third step (i.e. user might think there was discontinuity, even though in reality there wasn't).

@JanosJiri Let me know your thoughts. I am happy to merge this as is, and we can improve upon this later. 🚀

First of all, I agree it is not ideal but there shouldn't be a big problem. The problem is that it won't allow hopping the 2nd step of simulation (not 3rd, 3rd is propagation after hopping) because it has no memory of second derivatives. This should not be a problem when launching the simulations, because in LZ, you should not hop so early, the theory isn't derived for that (it assumes long propagation in the coupling region). The only problem might occur when restarting simulation.

I would merge it and later we can add the second derivative memory to the restart files, which is the only problem I see.

danielhollas

Okay, sounds good, thanks @JanosJiri

codecov · 2023-07-31T21:45:17Z

Codecov Report

Merging #159 (1d1d841) into master (7fba1ee) will decrease coverage by 0.03%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
- Coverage   92.39%   92.37%   -0.03%     
==========================================
  Files          43       43              
  Lines        6065     6084      +19     
  Branches      735      741       +6     
==========================================
+ Hits         5604     5620      +16     
- Misses        449      452       +3     
  Partials       12       12

Flag	Coverage Δ
unittests	`24.67% <0.00%> (-0.09%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/landau_zener.F90	`90.03% <95.23%> (-0.63%)`	⬇️
src/abin.F90	`93.12% <100.00%> (+0.16%)`	⬆️
src/force_abin.F90	`93.95% <100.00%> (+0.03%)`	⬆️
src/force_terash.F90	`97.02% <100.00%> (+0.01%)`	⬆️

danielhollas · 2023-07-31T21:50:39Z

Ugh, I thought I had already merged this. Merging now, apologies for the delay.

JanosJiri added 2 commits April 6, 2023 17:43

LZ hopping probabily evaluated only to neighbouring states

a162548

Current implementation allows for hops like (05-->01), which is not compatible with the LZ formula that was derived for two-state problem. This commit changes it and evaluates it only for closest neighbours.

JanosJiri requested review from danielhollas and suchanj April 12, 2023 07:13

danielhollas added 7 commits April 12, 2023 14:45

GHA: Update Ubuntu version to 20.04

14749f6

This unfortunately means we can no longer test with gfortran-7.

Fix gcc versions

5ac7a49

More fixes

ad668cb

FFTW tests seems to work now

5d0e95a

include OS version in cache keys

f77a531

Update Plumed versions

802d7e3

Try if GCC9 is available in Ubuntu 22.04

86a7cd1

danielhollas requested changes Apr 12, 2023

View reviewed changes

Comment thread src/abin.F90

Comment thread src/landau_zener.F90 Outdated

Comment thread src/landau_zener.F90 Outdated

Comment thread src/landau_zener.F90 Outdated

Comment thread src/landau_zener.F90

Comment thread src/landau_zener.F90

danielhollas added 18 commits April 12, 2023 16:37

Update actions/checkout

9fa909a

codecov: Update number of builds

d92acbc

verbose codecov

1df8765

Trying to fix codecov gcov paths

3899eae

do not install gcov

7e0d57f

Run gcov manually

42ccabf

Remove verbose

f2034d9

revert codecov.yml changes that did not do anything

8ea5179

simplify codecov yaml

b6680cc

Add gcov options to fix coverage?

de71492

Install gcc-7

9af96a7

Try again

5794619

Use gcov args as in codecov

2ab40ba

Install g++-7

bd1ccf1

Only install gcc-7 when needed

0978931

Try without codecov arguments

9348290

Back to 18 builds

effa476

GCC-7 with mpich

f1c63f6

danielhollas and others added 5 commits April 15, 2023 19:09

Disable most tests

9c1d760

Remove disabled macro

39dc950

Addressing comments of DH from previous pull request

c507356

The only significant change is a new condition for discontinuities.

Merge pull request #160 from PHOTOX/gha-ubuntu-update

70a1327

GHA: Update Ubuntu version to 20.04

Merge branch 'master' into lzsh

01a6662

JanosJiri requested a review from danielhollas April 17, 2023 16:07

danielhollas requested changes Apr 17, 2023

View reviewed changes

Comment thread src/landau_zener.F90

danielhollas force-pushed the master branch from 70a1327 to 5bee87b Compare April 17, 2023 21:04

Correcting typo and modified test reference for LZ_SS

01a7889

danielhollas changed the title ~~Improvements of LZSH algortihm~~ Improvements of LZSH algorithm May 23, 2023

Merge branch 'master' into lzsh

1d1d841

danielhollas requested review from danielhollas and removed request for suchanj July 27, 2023 13:25

danielhollas approved these changes Jul 27, 2023

View reviewed changes

danielhollas merged commit 542b515 into master Jul 31, 2023

danielhollas deleted the lzsh branch July 31, 2023 21:53

Conversation

JanosJiri commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielhollas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielhollas commented Apr 18, 2023

Uh oh!

JanosJiri commented Apr 19, 2023

Uh oh!

danielhollas commented May 22, 2023

Uh oh!

JanosJiri commented May 25, 2023

Uh oh!

danielhollas commented May 25, 2023

Uh oh!

danielhollas commented Jun 26, 2023

Uh oh!

JanosJiri commented Jun 27, 2023

Uh oh!

danielhollas commented Jun 27, 2023

Uh oh!

JanosJiri commented Jul 14, 2023

Uh oh!

danielhollas commented Jul 19, 2023

Uh oh!

JanosJiri commented Jul 27, 2023

Uh oh!

danielhollas left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 31, 2023

Codecov Report

Uh oh!

danielhollas commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JanosJiri commented Apr 12, 2023 •

edited

Loading