-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implementation of integrals over Gaussian orbitals #87
Conversation
|
@@ -0,0 +1,38 @@ | |||
name: unit tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, all non-blockers, as I've known this code from before.
I wonder if it might be better to move the folder e.g.
pyscf-ipu/pyscf_ipu/experimental -> pyscf-ipu/experimental/jax-integrals
|
||
for idx in range(n): | ||
for jdx in range(idx + 1): | ||
for kdx in range(idx + 1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct, or to be changed to j?
for kdx in range(idx + 1): | |
for kdx in range(jdx + 1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this needs to be left as idx
as these loops generate the canonicalised i, j, k, l
indices that handles the 8-fold permutation symmetries. To be honest, I was following the description in the four-index transformations book chapter - I think I understand that this is effectively pruning the complete 4-d mesh based on considering associativity between pairs. Put another way, the edit changing idx
-> jdx
generates the following 5 indices:
>>> list(gen_ijkl_edit(2))
[(0, 0, 0, 0), (1, 0, 0, 0), (1, 1, 0, 0), (1, 1, 1, 0), (1, 1, 1, 1)]
which is missing the (1, 0, 1, 0) index.
>>> list(gen_ijkl(2))
[(0, 0, 0, 0), (1, 0, 0, 0), (1, 0, 1, 0), (1, 1, 0, 0), (1, 1, 1, 0), (1, 1, 1, 1)]
@chex.dataclass | ||
class Basis: | ||
orbitals: Tuple[Orbital] | ||
structure: Structure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Probably not for this PR but...] Duplicated information here, as atom positions are both in orbitals and structure - how far coudl we get if we explicitly convert from Structure -> Basis and don't maintain the handy back-pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have thought about removing the structure
field from the Basis
class as you point out the atom positions are redundantly stored with in the orbital / primitives. The trouble is we need to know the structure for computing the occupied orbitals. Definitely needs some more thought!
Co-authored-by: Andrew Fitzgibbon <[email protected]>
I'd like to leave this until it becomes clearer how much we are going to build into the experimental module. Either at some point functionality here stops becoming experimental and is moved into a top-level namespace or it stops being useful and we might as well delete it right? |
This patch adds the following objects:
Primitive
: dataclass for a primitive Gaussian type orbitalOrbital
: dataclass for a linear combination ofPrimitive
, these are sometimes called contractions in the literature.Basis
: dataclass ofOrbital
s with an associated factory functionbasisset
that can read from the basis-set-exchange database.Structure
: dataclass for storing molecular structure (geometry and atom types)Adds implementations as derived in Gaussian-expansion methods for molecular integrals for the following:
All these integrals are implemented at the
Primitive
level and the results from these are used to build up the result of integration over the entireBasis
.Adds the following dependencies:
Also adds a github action for running tests using the jax cpu backend by default and a few tests that run on the IPU model. Note that the ERI tests cannot be tested on the github action runners due to requiring more than the 7GB of memory available.