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

Generates tables of indices and matrix elements to build electronic Hamiltonians #809

Merged
merged 29 commits into from Sep 23, 2020

Conversation

agran2018
Copy link
Contributor

@agran2018 agran2018 commented Sep 17, 2020

Context:
This PR extends PennyLane-QChem functionalities to construct observables of many-body quantum systems.

Description of the Change:
The functions one_particle and two_particle have been implemented to construct the tables of indices and
matrix elements of arbitrary one- and two-particle operators, respectively. These functions
can be used in conjunction with the observable function to construct electronic hamiltonians.
For example, in the case of a molecular Hamiltonian:

from pennylane import qchem
from openfermion.hamiltonians import MolecularData

# load the HF elect. structure
hf_data = MolecularData(filename="./lih")

# create lists with the indices and matrix elements of the one- and two-particle operators
t, t_core = qchem.one_particle(hf_data.one_body_integrals, core=[0], active=[1, 2])
v, v_core = qchem.two_particle(hf_data.two_body_integrals, core=[0], active=[1, 2])

# build the second-quantized Hamiltonian and map it to the qubit basis
H = qchem.observable([t, v], init_term=t_core+v_core)

Benefits:
These functions allow to build any electronic Hamiltonian based on the representations of the one- and two-particle operators in the basis of single-particle states.

Possible Drawbacks:
None

Related GitHub Issues:
None

@agran2018 agran2018 added review-ready 👌 PRs which are ready for review by someone from the core team. qchem ⚛️ Related to the QChem package labels Sep 17, 2020
@agran2018 agran2018 self-assigned this Sep 17, 2020
@codecov
Copy link

codecov bot commented Sep 17, 2020

Codecov Report

Merging #809 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #809      +/-   ##
==========================================
+ Coverage   90.64%   90.72%   +0.08%     
==========================================
  Files         127      127              
  Lines        8275     8347      +72     
==========================================
+ Hits         7501     7573      +72     
  Misses        774      774              
Impacted Files Coverage Δ
qchem/pennylane_qchem/qchem/obs.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99bd629...eedceda. Read the comment docs.

@@ -444,10 +444,308 @@ def particle_number(orbitals, mapping="jordan_wigner", wires=None):
return observable([table], mapping=mapping, wires=wires)


def one_particle(t_matrix_elements, core=None, active=None, cutoff=1.0e-12):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid using more than one underscore ( _ ) when naming!
For example, here, we can rename the argument to just matrix_elements. It's clear from context (the function is called one-particle) that these are elements for one-particle operators

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I added the t_ prefix was to differentiate from the matrix_elements argument of the observable function. But I agree that it must be clear from the docstring.

.. math::

\hat{T} = \sum_{\alpha, \beta} \langle \alpha \vert \hat{t} \vert \beta \rangle
[\hat{c}_{\alpha\uparrow}^\dagger \hat{c}_{\beta\uparrow} +
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the meaning of the arrows in alpha and beta? I found this confusing while reading the docstring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is explained just below in the docstring.
"the spin quantum numbers are indicated explicitly with the up/down arrows."
This notation is very convenient to stress that these operators do not act on the spin degrees of freedom and also to read the code.
See here https://pennylane--809.org.readthedocs.build/en/809/code/api/pennylane_qchem.qchem.obs.one_particle.html

qchem/pennylane_qchem/qchem/obs.py Outdated Show resolved Hide resolved
\hat{c}_{\alpha\downarrow}^\dagger \hat{c}_{\beta\downarrow}].

In the equation above the indices :math:`\alpha, \beta` run over the basis of spatial
orbitals :math:`\vert \alpha \rangle = \phi_\alpha(r)`. Since the operator :math:`t`
Copy link
Contributor

Choose a reason for hiding this comment

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

This equation |alpha> = phi(r) doesn't make sense, since we are equating two different mathematical objects, namely a vector and a function. We could either use <r|alpha> or, what I think is better, say "spatial orbitals |alpha> with associated wavefunctions phi(r)"


If an active space is defined (see :func:`~.active_space`), the summation indices
run over the active orbitals and the contribution due to core orbitals is computed as
:math:`T_\mathrm{core} = 2 \sum_{\alpha\in \mathrm{core}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a sum over alpha and beta? Currently only sum over alpha

Copy link
Contributor Author

@agran2018 agran2018 Sep 22, 2020

Choose a reason for hiding this comment

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

No. This is correct except that there is a typo!, thanks!. See the notes below.

20200922_122631

)

# Indices of the matrix elements with absolute values >= cutoff
indices = np.nonzero(np.abs(t_matrix_elements) >= cutoff)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!


In the equation above the indices :math:`\alpha, \beta, \gamma, \delta` run over the basis
of spatial orbitals :math:`\vert \alpha \rangle = \phi_\alpha(r)`. Since the operator
:math:`v` acts only on the spatial the spin quantum numbers are indicated explicitly
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is missing a part, it now reads "acts only on the spatial the spin quantum...". I guess it should be "acts only on the spatial orbitals, the spin quantum..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!, thanks @ixfoduap

\hat{c}_{\beta\downarrow}^\dagger \hat{c}_{\gamma\downarrow} \hat{c}_{\delta\downarrow}~].

In the equation above the indices :math:`\alpha, \beta, \gamma, \delta` run over the basis
of spatial orbitals :math:`\vert \alpha \rangle = \phi_\alpha(r)`. Since the operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above regarding |alpha> = phi

\langle \alpha, \beta \vert \hat{v} \vert \alpha, \beta \rangle`.

Args:
v_matrix_elements (array[float]): 4D NumPy array with the matrix elements
Copy link
Contributor

Choose a reason for hiding this comment

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

One question for me is: where is the user supposed to get access to these matrix elements? Is this a standard output from quantum chemistry packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It is not straightforward to get them printed out but there is always a possibility. In the particular cases of PySCF and Psi4 this is straightforward thanks to the interfaces with OpenFermion.


**Example**

>>> v_matrix_elements = np.array([[[[ 6.82389533e-01, -1.45716772e-16],
Copy link
Contributor

Choose a reason for hiding this comment

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

remove underscores from this example usage :-)

not correlated in the many-body wave function
active (list): indices of active orbitals, i.e., the orbitals used to
build the correlated many-body wave function
cutoff (float): Cutoff value for including matrix elements. The
Copy link
Contributor

Choose a reason for hiding this comment

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

Cutoff might be changed to cutoff for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

)
)

# Compute contribution due to core orbitals
Copy link
Contributor

@soranjh soranjh Sep 22, 2020

Choose a reason for hiding this comment

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

Is this comment necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it helps reading the code. If it is not a violation of the guidelines I would vote for keeping it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to keep, but usually the need for comments is a sign that the code could be made clearer, so as to not need comments

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to keep, but usually the need for comments is a sign that the code could be made clearer, so as to not need comments

Completely tangential, but while this might be the case in a lot of 'commodity' code, I find it's usually the opposite with scientific coding. Scientific code always has that extra layer of needing to interpret (potentially highly complex) equations, and implement them in a highly optimized manner.

So I always welcome commenting like this one here 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Scientific codes without comments are very difficult to read/maintain. This is the case of GAMESS, for example. More recent packages are much easier to develop.

Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
Copy link
Contributor Author

@agran2018 agran2018 left a comment

Choose a reason for hiding this comment

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

Thanks @ixfoduap for all suggestions!. Left few notes answering some of your questions.

.. math::

\hat{T} = \sum_{\alpha, \beta} \langle \alpha \vert \hat{t} \vert \beta \rangle
[\hat{c}_{\alpha\uparrow}^\dagger \hat{c}_{\beta\uparrow} +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is explained just below in the docstring.
"the spin quantum numbers are indicated explicitly with the up/down arrows."
This notation is very convenient to stress that these operators do not act on the spin degrees of freedom and also to read the code.
See here https://pennylane--809.org.readthedocs.build/en/809/code/api/pennylane_qchem.qchem.obs.one_particle.html


If an active space is defined (see :func:`~.active_space`), the summation indices
run over the active orbitals and the contribution due to core orbitals is computed as
:math:`T_\mathrm{core} = 2 \sum_{\alpha\in \mathrm{core}}
Copy link
Contributor Author

@agran2018 agran2018 Sep 22, 2020

Choose a reason for hiding this comment

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

No. This is correct except that there is a typo!, thanks!. See the notes below.

20200922_122631

not correlated in the many-body wave function
active (list): indices of active orbitals, i.e., the orbitals used to
build the correlated many-body wave function
cutoff (float): Cutoff value for including matrix elements. The
Copy link
Contributor Author

Choose a reason for hiding this comment

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

)
)

# Compute contribution due to core orbitals
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it helps reading the code. If it is not a violation of the guidelines I would vote for keeping it.


In the equation above the indices :math:`\alpha, \beta, \gamma, \delta` run over the basis
of spatial orbitals :math:`\vert \alpha \rangle = \phi_\alpha(r)`. Since the operator
:math:`v` acts only on the spatial the spin quantum numbers are indicated explicitly
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!, thanks @ixfoduap

\langle \alpha, \beta \vert \hat{v} \vert \alpha, \beta \rangle`.

Args:
v_matrix_elements (array[float]): 4D NumPy array with the matrix elements
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It is not straightforward to get them printed out but there is always a possibility. In the particular cases of PySCF and Psi4 this is straightforward thanks to the interfaces with OpenFermion.

)
)

# Compute contribution due to core orbitals
Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to keep, but usually the need for comments is a sign that the code could be made clearer, so as to not need comments

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Hi @agran2018 --- looks great! I didn't do a thorough review however, just left comments on the parts that stood out to me. Happy for this to be merged once the other reviewers have approved.

)
)

# Compute contribution due to core orbitals
Copy link
Member

Choose a reason for hiding this comment

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

It's fine to keep, but usually the need for comments is a sign that the code could be made clearer, so as to not need comments

Completely tangential, but while this might be the case in a lot of 'commodity' code, I find it's usually the opposite with scientific coding. Scientific code always has that extra layer of needing to interpret (potentially highly complex) equations, and implement them in a highly optimized manner.

So I always welcome commenting like this one here 🙂

[ 2. 2. -1.12554473]
[ 3. 3. -1.12554473]]
>>> print(t_core)
-9.45478626
Copy link
Member

Choose a reason for hiding this comment

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

Great docstring @agran2018!

)

# Compute contribution due to core orbitals
t_core = 2 * sum([matrix_elements[alpha, alpha] for alpha in core])
Copy link
Member

Choose a reason for hiding this comment

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

This looks like something that can be vectorized :)

not entirely sure what it would look like (you might have to play around with it), but something like the following:

2 * np.sum(matrix_elements[np.array(core), np.array(core)])

(some other permutation of the indexing might be required)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @josh146, this works fine.

)

# Indices of the matrix elements with absolute values >= cutoff
indices = np.nonzero(np.abs(matrix_elements) >= cutoff)
Copy link
Member

Choose a reason for hiding this comment

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

nice use of numpy functions!

Comment on lines +564 to +585
pairs = [
[indices[0][i], indices[1][i]]
for i in range(num_indices)
if all(indices[j][i] in active for j in range(len(indices)))
]

# Building the table of indices and matrix elements
table = np.zeros((2 * len(pairs), 3))

for i, pair in enumerate(pairs):
alpha, beta = pair
element = matrix_elements[alpha, beta]

# spin-up term
table[2 * i, 0] = 2 * active.index(alpha)
table[2 * i, 1] = 2 * active.index(beta)
table[2 * i, 2] = element

# spin-down term
table[2 * i + 1, 0] = 2 * active.index(alpha) + 1
table[2 * i + 1, 1] = 2 * active.index(beta) + 1
table[2 * i + 1, 2] = element
Copy link
Member

Choose a reason for hiding this comment

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

another potential vectorization 😆 It will probably be very ugly though, so it depends on the speed up needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point we have already dropped the negligible matrix elements and selected only a few active orbitals. I don't think it is critical here and keeping it explicitly will be helpful to remember the spin index mappings.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thanks @agran2018!

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Looks awesome! I left some very minor/optional comments.

qchem/pennylane_qchem/qchem/obs.py Outdated Show resolved Hide resolved
qchem/pennylane_qchem/qchem/obs.py Outdated Show resolved Hide resolved
if core is None:
t_core = 0
else:
if True in [i > orbitals - 1 or i < 0 for i in core]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing: could it be any([i > orbitals - 1 or i < 0 for i in core]) which might be easier to read

qchem/pennylane_qchem/qchem/obs.py Outdated Show resolved Hide resolved
agran2018 and others added 2 commits September 23, 2020 17:35
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
@agran2018 agran2018 merged commit 79d50e7 into master Sep 23, 2020
@agran2018 agran2018 deleted the qchem-table-matrix-elements branch September 23, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qchem ⚛️ Related to the QChem package review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants