Fix the AngularMomentum (again)
#1292
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this fix, the `AngularMomentum` operator was working on the assumption that the number of alpha-spin particles would always exceed the number of beta-spin particles (for non-singlet systems). However, this is not guaranteed to be the case. This commit fixes this problem by using a symmetric formula for S^2 which is the average of the two cases (alpha- vs. beta-spin particles exceeding the other).
When dealing with active spaces obtained from unrestricted-spin orbitals, the alpha-beta overlap matrix which is used to construct the S^2 operator can become non-unitary (for example, when the active set of alpha- and beta-spin orbitals do not span the same space). This can result in an <S^2> value measured on the active space that is largely different from (e.g.) the UHF <S^2> value. More importantly, when this is the case, the difference between these two <S^2> values is "hidden" in the inactive+inactive and inactive+active interactions. Especially when spin contamination is present, this can lead to vastly unexpected results and may indicate a poor choice of active space.
mrossinek
force-pushed
the
fix-overlap-anti-commutation
branch
from
528f24b to
531bb81
Compare
mrossinek
requested review from
woodsp-ibm,
robertodr and
matteoacrossi
as code owners
Pull Request Test Coverage Report for Build 7083799155
💛 - Coveralls |
robertodr
reviewed
Dec 4, 2023
| """ | ||
| self.num_spatial_orbitals = num_spatial_orbitals | ||
| self._overlap: np.ndarray | None = None | ||
| self.overlap = overlap |
There was a problem hiding this comment.
For my education: isn't this overridden by the overlap property?
There was a problem hiding this comment.
I see it now (I think! 😺) this line will call the setter and apply the initialization logic. But then is there a point to type-hint self._overlap a union type? Couldn't it be just np.ndarray?
There was a problem hiding this comment.
The first line here makes sure that pylint does not complain about the private _overlap attribute not being defined inside the __init__ (because otherwise it would be defined inside the property method.
I am typing it for the sake of completeness 👍
robertodr
reviewed
Dec 4, 2023
| self.overlap = overlap | ||
|
|
||
| @property | ||
| def overlap(self) -> np.ndarray | None: |
There was a problem hiding this comment.
Is it ever possible for this to return None?
There was a problem hiding this comment.
Yes, this can be left as None in which case the methods will internally treat it as an identity matrix of the correct size (but there is no need to store that square matrix so I left it as None 👍)
robertodr
approved these changes
Dec 4, 2023
mrossinek
added
the
stable backport potential
mergify bot
pushed a commit
that referenced
this pull request
Dec 4, 2023
* fix: use the symmetric formula for S^2 Prior to this fix, the `AngularMomentum` operator was working on the assumption that the number of alpha-spin particles would always exceed the number of beta-spin particles (for non-singlet systems). However, this is not guaranteed to be the case. This commit fixes this problem by using a symmetric formula for S^2 which is the average of the two cases (alpha- vs. beta-spin particles exceeding the other). * feat: log a warning when the alpha-beta overlap is non-unitary When dealing with active spaces obtained from unrestricted-spin orbitals, the alpha-beta overlap matrix which is used to construct the S^2 operator can become non-unitary (for example, when the active set of alpha- and beta-spin orbitals do not span the same space). This can result in an <S^2> value measured on the active space that is largely different from (e.g.) the UHF <S^2> value. More importantly, when this is the case, the difference between these two <S^2> values is "hidden" in the inactive+inactive and inactive+active interactions. Especially when spin contamination is present, this can lead to vastly unexpected results and may indicate a poor choice of active space. * Add reno * test: add a regression test * fix: update warning tolerance * Linting and formatting * docs: fix verbatim in RST (cherry picked from commit 55e6776)
mergify bot
added a commit
that referenced
this pull request
Dec 4, 2023
* fix: use the symmetric formula for S^2 Prior to this fix, the `AngularMomentum` operator was working on the assumption that the number of alpha-spin particles would always exceed the number of beta-spin particles (for non-singlet systems). However, this is not guaranteed to be the case. This commit fixes this problem by using a symmetric formula for S^2 which is the average of the two cases (alpha- vs. beta-spin particles exceeding the other). * feat: log a warning when the alpha-beta overlap is non-unitary When dealing with active spaces obtained from unrestricted-spin orbitals, the alpha-beta overlap matrix which is used to construct the S^2 operator can become non-unitary (for example, when the active set of alpha- and beta-spin orbitals do not span the same space). This can result in an <S^2> value measured on the active space that is largely different from (e.g.) the UHF <S^2> value. More importantly, when this is the case, the difference between these two <S^2> values is "hidden" in the inactive+inactive and inactive+active interactions. Especially when spin contamination is present, this can lead to vastly unexpected results and may indicate a poor choice of active space. * Add reno * test: add a regression test * fix: update warning tolerance * Linting and formatting * docs: fix verbatim in RST (cherry picked from commit 55e6776) Co-authored-by: Max Rossmannek <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1291
Details and comments
See also #1291 (comment) for some more details.