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

Solver convergence #175

Merged
merged 17 commits into from
Oct 23, 2024

Conversation

apapapostolou
Copy link
Contributor

@apapapostolou apapapostolou commented Aug 8, 2024

Several people have encountered the problem that the adcc results don’t always match the Qchem results, i.e., excitation energies already deviate in the second digit. Sometimes this only occurs with higher excited states, but we have also had some cases where the first excited states were already affected.

We (@AdrianLDempwolff and @Drrehn) have found that the problem has two different causes, both of which result in the solver procedure converging far less than it should.

  1. When checking for convergence, the squared residual norm is considered and not the actual residual norm. According to a comment in davidson.py, this is done because convergence is also checked this way in Qchem, but this is actually not the case. We have checked the Qchem code.
  2. If Pyscf is used as the backend, the convergence tolerance of the reference state is determined as the maximum of 10 * conv_tol and conv_tol_grad in pyscf.py. However, since conv_tol_grad is calculated in Pyscf as the square root of conv_tol, it should rather be the maximum of conv_tol and conv_tol_grad**2. This results in the imported convergence tolerance being much greater than it actually is.

A brief example will illustrate the consequences of these two points in numbers: If the SCF is converged to 1e-9 with Pyscf (which is the default), this is imported into adcc as reference_state.conv_tol = sqrt(1e-9) = 3.2*1e-5 (see pyscf.py). In workflow.py, the convergence tolerance for the Davidson is determined from this as conv_tol = max(10 * reference_state.conv_tol, 1e-6) = 3.2*1e-4. In davidson.py, the square of the residual norm is then calculated instead of the residual norm, which means that the actual convergence is only sqrt(3.2*1e-4)= 3.2*1e-2, which is clearly too large. In Qchem, the default convergence tolerance is 1e-6.

This pull request fixes these two problems.

A minor point is that the convergence tolerance of the reference state is multiplied by a factor of 10 during import and multiplied again by a factor of 10 when determining the default solver convergence tolerance, but shouldn't one time be enough?

@maxscheurer
Copy link
Member

Some of the test failures are of course related to "more iterations" in the Davidson procedure... The mismatches wrt properties/energies are harder to fix probably... 😦

@maxscheurer
Copy link
Member

Should be updated against current master, then the pipeline will run again.

@apapapostolou
Copy link
Contributor Author

A minor point is that the convergence tolerance of the reference state is multiplied by a factor of 10 during import and multiplied again by a factor of 10 when determining the Davidson convergence tolerance, but shouldn't one time be enough? I have not yet addressed this last point.

@AdrianLDempwolff what do you think about this? How is it done in Q-Chem?

@apapapostolou
Copy link
Contributor Author

@maxscheurer here you can find the updated adcc reference data: https://heibox.uni-heidelberg.de/d/cf184e9f2de54bb58683/

@maxscheurer
Copy link
Member

@apapapostolou the data are uploaded, you need to change the download URL to 0.6.0 and get the hopefully correct SHA256SUMS file here: https://q-chem.de/adcc_testdata/0.6.0/SHA256SUMS

@apapapostolou apapapostolou marked this pull request as ready for review October 23, 2024 10:13
@maxscheurer
Copy link
Member

I think you need to rebase against master before being able to merge, @apapapostolou.

@apapapostolou apapapostolou merged commit 2060a8d into adc-connect:master Oct 23, 2024
8 checks passed
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.

3 participants