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

Kappa minus one #491

Merged
merged 12 commits into from
Sep 22, 2024
Merged

Kappa minus one #491

merged 12 commits into from
Sep 22, 2024

Conversation

ilfreddy
Copy link
Contributor

@ilfreddy ilfreddy commented Aug 15, 2024

The current ZORA implementation has a major flaw. The $\kappa$ function is projected "as is", which means that its precision is significantly reduced, because the function is essentially one in the whole volume and has cusps in the neighbourhood of each nucleus. As such the function is strictly speaking not an L2 function. We can still represent it because the simulation box is finite. However its norm gets basically as large as the volume, affecting the actual precision of the representation of the cusps. This will in turn affect also the representation of derivatives (zero everywhere except at the nuclei)

The function $\kappa - 1$ is therefore a much better choice.

This PR implements the projection of $\chi = \kappa - 1$. Whenever $\kappa$ is required, we use $\chi + 1$ instead.

Notice also that $\nabla \kappa = \nabla \chi$. Derivatives are therefore unaffected and (hopefully) computed at a better precision.

@moritzgubler
Copy link
Contributor

The changes we made would solve both the examples (NH3 and H2 with tight precisions) that did not converge in issue #474. I brought the branch up to date with master, added of documentation and cleaned up the code. @ilfreddy I think you can remove the draft status, the PR is ready for review now in my opinion.

@ilfreddy ilfreddy marked this pull request as ready for review September 3, 2024 11:46
Copy link
Contributor

@stigrj stigrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failing test seems to be the same issue as #487, so I will just bypass the test requirement and merge this

@stigrj stigrj merged commit f258b45 into MRChemSoft:master Sep 22, 2024
7 of 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