-
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
[WIP] Adds decomposition to DoubleExcitation
operator (and update gradient recipes)
#1303
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1303 +/- ##
=======================================
Coverage 98.15% 98.15%
=======================================
Files 148 148
Lines 11357 11359 +2
=======================================
+ Hits 11147 11149 +2
Misses 210 210
Continue to review full report at Codecov.
|
DoubleExcitation
operator.DoubleExcitation
operator (and update gradient recipes)
…nyLaneAI/pennylane into decompose_double_excitations
pennylane/ops/qubit.py
Outdated
@@ -1707,14 +1707,13 @@ class SingleExcitation(Operation): | |||
|
|||
* Number of wires: 2 | |||
* Number of parameters: 1 | |||
* Gradient recipe: Obtained from its decomposition in terms of the | |||
:class:`~.SingleExcitationPlus` and :class:`~.SingleExcitationMinus` operations | |||
* Gradient recipe: The SingleExcitation operator satisfies a four-term parameter-shift rule |
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.
@dwierichs just to confirm, this is what you had in mind for enabling both operations to use the four-term shift rules?
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, exactly :)
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! 💯 🥇 this is very helpful, thank you for going so far to get this one! 🎉
* The `qml.SingleExcitation` and `qml.DoubleExcitation` operations now | ||
have decompositions over elementary gates, and their gradient recipes | ||
have been updated to use the four-term parameter-shift rules. | ||
[(#1303)](https://github.com/PennyLaneAI/pennylane/pull/1303) |
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.
🥳 🥳 🥳
mats = [] | ||
for i in reversed(decomp): | ||
if i.wires.tolist() == [1, 0] and isinstance(i, qml.CRY): | ||
new_mat = np.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.
(minor): We could potentially have a separate function e.g., cry_target_first
in the gate_data.py
file that could be called here and in the other test cases. This is out of scope, as it means a bit of organization also for test_single_excitation_plus_decomp
and test_single_excitation_minus_decomp
.
This would just be a step towards making this file slightly more organized, as the matrix representation of controlled-operations where the wires are in a non-ascending order start coming up in the tests for decompositions.
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.
That would be super useful to have just in general. We could actually have something very generic, like:
def non_adjacent_controlled_mat(U, wires, num_wires):
...
And then return the matrix where C-U is applied to wires
in a num_wires
-qubit system. (Basically, what I have implemented in the DoubleExcitation
test; and similarly for a random single-qubit gate sitting on a qubit amidst other qubits, if that doesn't already exist somewhere).
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 will make an issue for this so we don't forget to address it in the future 👍
tests/ops/test_qubit_ops.py
Outdated
P0 = np.array([[1, 0], [0, 0]]) | ||
P1 = np.array([[0, 0], [0, 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.
How about moving these into gate_data
and maybe naming them state_zero_projector
and state_one_projector
?
The pro is that it'll make the test shorter, although it will also be less explicit. So if it would look less readable (as the names are not descriptive enough), then leaving this as is the way to go.
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 didn't know about gate_data
😅 that's a good idea. These projectors are quite commonly used, so I think it's worth having them in there. I will call them perhaps StateZeroProjector
and StateOneProjector
to match the existing capitalization conventions if that works.
Co-authored-by: antalszava <antalszava@gmail.com>
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 great! @glassnotes: you make this look so easy! 🧙
decomp_ops = [ | ||
DoubleExcitationPlus(theta / 2, wires=wires), | ||
DoubleExcitationMinus(theta / 2, wires=wires), | ||
qml.CNOT(wires=[wires[2], wires[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.
Yikes! Makes this building block look less like a fundamental one 😅
Still, not too bad in the end!
decomposed_matrix = mats[0] @ mats[1] | ||
"""Tests that the SingleExcitation operation calculates the correct decomposition. | ||
|
||
Need to consider the matrix of CRY separately, as the control is wire 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.
Why is this change to the single excitation gate being done in this PR? Just sneaking it in?
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's because we are updating the gradient recipes for both gates to the four-term shift rules; so the SingleExcitation
decomposition is adjusted accordingly.
|
||
from functools import reduce | ||
|
||
# To compute the matrix for CX on an arbitrary number of qubits, use the fact that |
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.
🧠
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
Context: Currently
DoubleExcitation
is decomposed intoDoubleExcitationPlus
andDoubleExcitationMinus
, neither of which are decomposed into elementary gates, such thatDoubleExcitation
cannot be implemented on all devices.Description of the Change: Adds the decomposition into elementary gates based on this paper to
DoubleExcitation
. Furthermore, updates the gradient recipe of bothSingleExcitation
andDoubleExcitation
to use the four-term parameter-shift rule of the aforementioned paper.Benefits:
DoubleExcitation
can now run on any device;Single
andDouble
excitation gradients are improved.Possible Drawbacks: None
Related GitHub Issues: #1275 #1278