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

Linear density delta kick and restart #2543

Merged
merged 1 commit into from Feb 3, 2023
Merged

Linear density delta kick and restart #2543

merged 1 commit into from Feb 3, 2023

Conversation

glb96
Copy link
Contributor

@glb96 glb96 commented Feb 1, 2023

No description provided.

@glb96
Copy link
Contributor Author

glb96 commented Feb 1, 2023

  • Fix some issues with the restart, especially for non-uniform occupation
  • Add the possibility to apply delta kick for density propagation. To do so use the same routine as for MO-based RTP. There is a temporary use of rtp%mos which is deallocated just after the delta kick use.
  • Add tests regarding the density propagation with delta kick and restart. The slow tests using Si8 are made for testing non-uniform occupation. May be changed to a faster system.
  • Small update of the input description

@glb96 glb96 marked this pull request as ready for review February 1, 2023 13:52
@glb96
Copy link
Contributor Author

glb96 commented Feb 1, 2023

I do not understand the problem with the regtest!

@oschuett
Copy link
Member

oschuett commented Feb 1, 2023

It seems you accidentally included a different DBCSR version.
Also, some of the new tests are quite slow. Could you please try to speed them up a bit?

@glb96
Copy link
Contributor Author

glb96 commented Feb 2, 2023

The dbcsr version should be the good one
I have increase some test speed.
Hope it works!

@glb96
Copy link
Contributor Author

glb96 commented Feb 2, 2023

This time the tests were all correct (including the new ones from me) but I still get a 'failed' error! Is it because it is too slow?

@oschuett
Copy link
Member

oschuett commented Feb 2, 2023

Is it because it is too slow?

Yes, unfortunately two of your new tests are still quite slow:

----------------------------- Slow Tests -------------------------------
Duration threshold (2x 95th %ile): 40.22 sec
Found 2 slow tests (29 suppressed):
    QS/regtest-rtp-5/si8-smearing-rtp-dens.inp                                       (  59.09 sec)
    QS/regtest-rtp-5/si8-smearing-rtp-dens-pulse-1.inp                               (  59.36 sec)

@glb96
Copy link
Contributor Author

glb96 commented Feb 2, 2023

Thx for your help!
I re-opened this pull request with a speed-up of the new tests. The time for the 3 slower tests has been almost cut in half.

@glb96 glb96 reopened this Feb 2, 2023
@glb96 glb96 marked this pull request as draft February 3, 2023 07:21
@glb96 glb96 marked this pull request as ready for review February 3, 2023 07:21
@glb96 glb96 closed this Feb 3, 2023
@glb96 glb96 reopened this Feb 3, 2023
@glb96
Copy link
Contributor Author

glb96 commented Feb 3, 2023

The new tests were fast enough, but I get some other tests too slow now... What should I do?
Is there a way to re-try the CI tests for the pull request?

@oschuett
Copy link
Member

oschuett commented Feb 3, 2023

Thanks for speeding up your tests. Don't worry about those other tests. I'll take care of it.

@oschuett oschuett merged commit 427e198 into cp2k:master Feb 3, 2023
@oschuett
Copy link
Member

oschuett commented Feb 6, 2023

There seems to be a problem at

CALL cp_fm_vect_dealloc(rtp%mos%next)

@glb96
Copy link
Contributor Author

glb96 commented Feb 6, 2023

I do not understand why this error was raised. I am looking to see how to fix this. Where/how was the regtests performed? Last time I tried on the github platform for this pull request the tests were fine (except for time-delay issues).

@oschuett
Copy link
Member

oschuett commented Feb 7, 2023

I do not understand why this error was raised.

What happened is that rt_prop_create_mos(init_mos_next=.FALSE.) left rtp%mos%next uninitialized:

IF (my_mos_next) ALLOCATE (rtp%mos%next(2*nspin))

Unfortunately, uninitialized memory it is only sometimes detected by tests. I fixed it by adding default initializes.

Where/how was the regtests performed?

We only run a few tests on pull requests to keep the cost low and the pace of development fast. Most of our tests run twice per day on our master branch. The results are published at https://dashboard.cp2k.org.

There are still two problems left:

  1. The threshold of H2O-wfn-mix-dens-pulse-1.inp seems a bit too tight:
/opt/cp2k/regtesting/TEST-local-psmp-2023-02-07_00-55-04/QS/regtest-rtp-5/H2O-wfn-mix-dens-pulse-1.inp.out
Difference too large: 1.39e-10 > 6e-11, ref_value: -16.80397942635466, value: -16.80397942868343.
  1. The minimal test found another problem. Unfortunately, Fortran does not use short circuit logic:

IF (ASSOCIATED(rtp%mos) .AND. ASSOCIATED(rtp%mos%old)) THEN

Could you please prepare a PR to address those two problems?

@glb96
Copy link
Contributor Author

glb96 commented Feb 7, 2023

Thank you for your help and your explications. I will fix these two issues and prepare a PR.

Just to be sure to understand the short-cut problem. Writing:
IF (var_a .AND. var_b) THEN
would be the same as
IF (var_a) THEN
IF (var_b) THEN
in the case where both var_a and var_b would be initialized/defined.
The problem is that for most Fortran compilers using
IF (var_a .AND. var_b) THEN
evaluate var_a, and var_b while for some other (few cases) it only evaluates var_b if var_a is True?

@oschuett
Copy link
Member

oschuett commented Feb 7, 2023

IF (var_a .AND. var_b) THEN
evaluate var_a, and var_b while for some other (few cases) it only evaluates var_b if var_a is True?

Unfortunately, the Fortran standard does not define how such an expression have to be evaluated. With optimizations enabled the gfortran compiler will stop once it finds var_a to be false. However, without optimizations it will always evaluates both expressions and then compute the .AND. afterwards.

So in a case like IF (ASSOCIATED(rtp%mos) .AND. ASSOCIATED(rtp%mos%old)) THEN it tries to access rtp%mos even when it's not associated. The solutions is to use nested IF statements. It's ugly but safe.

This was referenced Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants