Open
Conversation
…rbitals, including those that are frozen
Collaborator
|
n_frozen should transform as n, which is multiplied by 2 if I am not mistaken? |
Schoyen
requested changes
Jun 2, 2020
Collaborator
Schoyen
left a comment
There was a problem hiding this comment.
Nice! I agree with @haakoek and @halvarsu that n_frozen in a GeneralOrbitalSystem should be twice the number of n_frozen in a SpatialOrbitalSystem when transforming from the latter to the former. This reflects the fact that n now acts as the number of occupied orbitals in any system and does not reflect the number of particles.
There are a few things in this pull request that needs to be addressed before it will work as intended (I think).
- The proposed changes does not pass on the number of frozen orbitals from a
SpatialOrbitalSystemto aGeneralOrbitalSystem. This means thatn_frozenis completely ignored inconstruct_pyscf_system_rhfifadd_spin=True. To remedy this you need to pass onn_frozen=self.n_frozen * 2as the last keyword argument here: https://github.com/Schoyen/quantum-systems/blob/0260d051540b9d1f2b164f50d64bf80c8b42b606/quantum_systems/spatial_orbital_system.py#L81-L85 - Add an optional
n_frozen=0argument toconstruct_pyscf_system_aoas well to make the two PySCF-methods behave in as similar manner as possible. - Add test of the usage of the
n_frozen-attribute. This test does not have to anything other than constructing a few different systems (bothSpatialOrbitalSystemandGeneralOrbitalSystem), e.g., from PySCF and/or fromRandomBasisSet. Make sure thatn_frozenis set to the expected number, and check that when transforming from aSpatialOrbitalSystemto aGeneralOrbitalSystemyou get a doubling inn_frozen.
Less important feedback:
I propose the convention n_frozen instead of nfrozen to better suit the snake-case naming used elsewhere in the code.
This file contains hidden or 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
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.
This pull requests allows for frozen core orbitals. This is implemented by modifying the slice for occupied orbitals:
self.o = slice(0,system.n) -> slice(nfrozen,system.n),
where nfrozen now is an argument to the constructor of the system class. The parameter nfrozen just gives the number of electrons to freeze, so it does not discriminate between restricted or general type systems. This has the potential pitfall that, for example nfrozen=1 effectively freezes the lowest lying doubly occupied orbital for a restricted system, while freezing just 1 spin-orbital for a general system. We have support for going from a restricted to a general system, thus we should make a choice if this automatically should mean that nfrozen is multiplied by 2 (I think this is most sensible, but maybe it is debatable).
Note that system.n, system.l and system.m are unchanged and that the number of virtual orbitals (self.m) is unaffected by the frozen core. However, it should be noted that when the reference energy and the fock matrix is constructed all occupied orbitals have to be used. This is handled explicitly in these methods.