-
Notifications
You must be signed in to change notification settings - Fork 575
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
Add k-UpCCGSD Template #1743
Add k-UpCCGSD Template #1743
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1743 +/- ##
=======================================
Coverage 98.91% 98.91%
=======================================
Files 206 207 +1
Lines 15564 15625 +61
=======================================
+ Hits 15395 15456 +61
Misses 169 169
Continue to review full report at Codecov.
|
spin-orbitals included in the active space, and should be even. | ||
|
||
#. The number of trainable parameters scales linearly with the number of layers as | ||
:math:`2 k ||\vec{\theta}||`, where :math:`||\vec{\theta}||` is the total number of |
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.
Maybe use a symbol like n
for representing the total number of excitation terms?
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 updated it. Does it look better now?
Args: | ||
k (int): Number of layers | ||
n_wires (int): Number of qubits | ||
delta_saz (int): Specifies the selection rules ``sz[p] - sz[r] = delta_sz`` |
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.
delta_saz (int): Specifies the selection rules ``sz[p] - sz[r] = delta_sz`` | |
delta_sz (int): Specifies the selection rules ``sz[p] - sz[r] = delta_sz`` |
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.
Fixed.
6, | ||
6, | ||
qml.math.array( | ||
[ |
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.
Are we sure the reference state is the correct one?
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.
Yes. I have computed them manually to cross-check.
], | ||
) | ||
def test_shape(self, k, n_wires, delta_sz, expected_shape): | ||
"""Test that the shape method returns the correct shape of the weights tensor""" |
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.
"""Test that the shape method returns the correct shape of the weights tensor""" | |
"""Test that the shape method returns the correct shape of the weights tensor.""" |
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.
Fixed.
|
||
|
||
class TestInterfaces: | ||
"""Tests that the template is compatible with all interfaces, including the computation |
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.
We should change "Tests" to "Test" for consistency, everywhere.
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 think so. I have made the suggested changes.
|
||
|
||
def circuit_decomposed(weights): | ||
qml.BasisState(np.array([0, 0, 1, 1]), wires=[0, 1, 2, 3]) |
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.
Any reason why the initial state is flipped?
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.
Mainly because I am using the same wire conventions as qchem.excitations_to_wires
. The state is flipped in the UCCSD
template as well, which I suppose is related to wire indices in SingleExcitationUnitary
and DoubleExcitationUnitary
.
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.
Does the test pass if we use np.array([1, 1, 0, 0])
?
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 just tested and it didn't pass. The expectation value Z0
returned for np.array([1, 1, 0, 0])
is opposite in sign in this case.
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.
Good catch Soran. Yes, this is a known issue with these old excitation unitaries. It's in fact one of the reasons the tutorial on VQE in different spin sectors had to be changed.
Another reason to revisit these old excitations!
from pennylane.operation import Operation, AnyWires | ||
|
||
|
||
def generalized_singles(wires, delta_sz): |
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 function tested?
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.
Thanks for pointing it out. I have now added a test for this and generalized_pair_doubles
.
return gen_singles_wires | ||
|
||
|
||
def generalized_pair_doubles(wires): |
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 function tested?
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.
Thanks for pointing it out. It is now. Look at my comment above.
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.
What is "generalized" here and in generalized_singles
?
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.
Looks good to me, left few suggestions and questions.
Co-authored-by: Jay Soni <jbsoni@uwaterloo.ca>
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.
Looks very good overall, just minor suggestions
@@ -178,6 +178,11 @@ Other useful templates which do not belong to the previous categories can be fou | |||
:description: UCCSD | |||
:figure: ../_static/templates/subroutines/uccsd.png | |||
|
|||
.. customgalleryitem:: | |||
:link: ../code/api/pennylane.templates.subroutines.kUpCCGSD.html | |||
:description: k-UpCCGSD |
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.
Small nitpick: Please change the kets with dots |.> on the left of the image with just dots
|0>
|0>
.
.
|1>
|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.
I have updated the image.
|
||
|
||
class kUpCCGSD(Operation): | ||
r"""Implements the k-Unitary pair Coupled-Cluster Generalized Singles and Doubles (k-UpCCGSD) ansatz. |
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.
r"""Implements the k-Unitary pair Coupled-Cluster Generalized Singles and Doubles (k-UpCCGSD) ansatz. | |
r"""Implements the k-Unitary Pair Coupled-Cluster Generalized Singles and Doubles (k-UpCCGSD) ansatz. |
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 corrected it.
r"""Implements the k-Unitary pair Coupled-Cluster Generalized Singles and Doubles (k-UpCCGSD) ansatz. | ||
|
||
The k-UpCCGSD ansatz calls the :func:`~.SingleExcitationUnitary` and :func:`~.DoubleExcitationUnitary` | ||
templates to exponentiate the product of :math:`k` generalized singles and pair coupled-cluster doubles |
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.
It may be good to explain what we mean by "generalized" here. @obliviateandsurrender You can use a more compact version of the message you sent me on slack
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.
NOTE: we should re-visit these excitation unitaries to understand if we can improve their implementation. Right now it is very slow
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 elucidated it in the text. Does it look sufficient?
single and pair double excitation terms. | ||
wires (Iterable): wires that the template acts on | ||
k (int): Number of times UpCCGSD unitary is repeated. | ||
delta_saz (int): Specifies the selection rules ``sz[p] - sz[r] = delta_sz`` |
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.
delta_saz (int): Specifies the selection rules ``sz[p] - sz[r] = delta_sz`` | |
delta_saz (int): Specifies the selection rule ``sz[p] - sz[r] = delta_sz`` |
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.
Updated it.
spin-orbitals included in the active space, and should be even. | ||
|
||
#. The number of trainable parameters scales linearly with the number of layers as | ||
:math:`2 k n`, where :math:`n = ||\vec{\theta}||` is the total number of |
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.
:math:`2 k n`, where :math:`n = ||\vec{\theta}||` is the total number of | |
:math:`2 k n`, where :math:`n` is the total number of |
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.
Updated it.
H, qubits = qml.qchem.molecular_hamiltonian(symbols, coordinates) | ||
|
||
# Define the Hartree-Fock state | ||
delta_sz = 0 |
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.
delta_sz = 0 |
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.
Updated this and its subsequent usages.
return gen_singles_wires | ||
|
||
|
||
def generalized_pair_doubles(wires): |
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.
What is "generalized" here and in generalized_singles
?
|
||
|
||
def circuit_decomposed(weights): | ||
qml.BasisState(np.array([0, 0, 1, 1]), wires=[0, 1, 2, 3]) |
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.
Good catch Soran. Yes, this is a known issue with these old excitation unitaries. It's in fact one of the reasons the tutorial on VQE in different spin sectors had to be changed.
Another reason to revisit these old excitations!
Context:
Added a new template
kUpCCGSD
, which implements a unitary coupled cluster ansatz with generalized singles and pair doubles excitation operators, proposed by Joonho Lee et al. in arXiv:1810.02327Description of the Change:
Adds the
kUpCCGSD
class implementing the template and the relevant test cases.Benefits:
Extends the currently available ansatz templates for quantum chemistry in PennyLane.
Possible Drawbacks:
No drawbacks.
Related GitHub Issues:
None.