-
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
Implement transforms to cancel redundant single-qubit operations #1455
Implement transforms to cancel redundant single-qubit operations #1455
Conversation
Hello. You may have forgotten to update the changelog!
|
…ps://github.com/PennyLaneAI/pennylane into ch7104-implement-transforms-to-cancel-redundant
Codecov Report
@@ Coverage Diff @@
## master #1455 +/- ##
==========================================
+ Coverage 98.23% 98.27% +0.03%
==========================================
Files 163 167 +4
Lines 12086 12242 +156
==========================================
+ Hits 11873 12031 +158
+ Misses 213 211 -2
Continue to review full report at Codecov.
|
@@ -44,7 +44,9 @@ | |||
:toctree: api | |||
|
|||
~adjoint | |||
~transforms.cancel_inverses |
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.
Once we have more compilation transforms available, these should go in their own section together.
are_angles_1_zero = allclose(angles_1, zeros(3)) | ||
are_angles_2_zero = allclose(angles_2, zeros(3)) | ||
|
||
if are_angles_1_zero and are_angles_2_zero: |
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.
This was a tricky function to write; I've separated things out as much as possible so that the quaternion math only has to be done when it's needed, but in principle we could make this a lot more streamlined by removing some of the cases. Not sure yet what is the best option; probably it'd be a good idea to do some more benchmarking/testing to determine whether having multiple cases like this is actually worth it.
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.
Halfway through my review, will complete the rest tomorrow 🙂
>>> print(qml.draw(optimized_qnode)()) | ||
0: ──T──┤ ⟨X⟩ | ||
1: ──Z──┤ |
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.
🤯 This is almost magical
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.
🧙♀️
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.
Agreed, amazing! 🤩
pennylane/operation.py
Outdated
is_self_inverse = None | ||
"""bool: True if the operation is its own inverse.""" | ||
|
||
is_symmetric_over_wires = None | ||
"""bool: True if the operation is the same if you exchange the order of wires.""" | ||
|
||
is_composable_rotation = None | ||
"""bool: True if the composing the operations results in an addition of parameters.""" |
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 might be good to have bool or None
, and to specify what None
means?
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.
Oh, and is_symmetric_over_wires
and is_composable_rotation
might benefit from a more detailed description/small example?
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.
After seeing that CZ
is symmetric-over-wires, I get it now 😆
|
||
|
||
@qfunc_transform | ||
def merge_rotations(tape): |
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 there any utility in allowing only specific rotation gates be selected?
e.g.,
@qml.qnode(dev)
@merge_rotations(tape, ops={"RX", "RY", "CRZ"})
def qfunc(params):
(can't think of a better argument name 🤔 )
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 this is a good idea; another suggestion from Nathan was to add an accuracy threshold for the angle cancellation.
I'd been wondering whether having a mix of parametrized and non-parametrized functions would make it more difficult to write the compile
function though? Or could the pipeline be specified and applied like this:
pipeline = [cancel_inverses, merge_rotations(ops=["RX", "CRZ"])]
for transform in pipeline:
qfunc = transform(qfunc)
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.
another suggestion from Nathan was to add an accuracy threshold for the angle cancellation.
Good idea!
Or could the pipeline be specified and applied like this:
Omg I think this would work! I've often despaired at transform(params)(func)(args)
being less-than-intuitive, but this might be an advantage 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.
Okay amazing, I will update this then!
|
||
# Queue the measurements normally | ||
for m in tape.measurements: | ||
apply(m) |
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 strikes me that so much of this for loop is similar to the previous transform... I wonder if there is a way of 'combining' the for loops if both are applied consecutively 🤔
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.
Oh another question - I suppose this transform would have to be run multiple times to merge all Rot gates?
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 should actually glob together arbitrary-length sequences, so if there 3 Rot
gates in a row, it will end up as a single Rot
in just one pass. Probably a good thing to add to a test!
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 strikes me that so much of this for loop is similar to the previous transform... I wonder if there is a way of 'combining' the for loops if both are applied consecutively
We could just combine them into a single transform, but I kind of like the flexibility of them being separate. Also, the way the loops are set up, you might run into problems with circuits like the following:
qml.RZ(0.3, wires=0)
qml.Hadamard(wires=0)
qml.Hadamard(wires=0)
qml.RZ(0.2, wires=0)
If we do only one pass-through of a combined version, the two Hadamards would cancel, but the two RZ
wouldn't merge, because we've moved ahead to the second RZ
and it won't see the one behind. To catch them both, we'd need a second pass, at which point we may as well have a separate transform.
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.
got it 👍
from pennylane.wires import Wires | ||
|
||
|
||
def _find_next_gate(wires, op_list): |
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.
since these functions are being imported elsewhere, it kinda feels like they shouldn't be private 🤔 To me, private functions are generally used within the same module they are defined (not sure if there is a strong Python convention for this or not)
return stack([z1, y, z2]) | ||
|
||
|
||
def _fuse_rot_angles(angles_1, angles_2): |
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.
Feel free to turn this off using a pylint comment (or, alternatively, it might be a sign it could be factored out into smaller unit functions)
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.
Tests are 💯 @glassnotes! I don't have any major suggestions, just minor ones I left in the first half of the review :)
|
||
# Then we can combine to create | ||
# RZ(a + u) RY(v) RZ(w + f) | ||
return stack([left_z + u, v, w + right_z]) |
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.
code looks good, but I must admit I did not check the mathematics!
|
||
new_tape = qml.transforms.make_tape(transformed_qfunc)() | ||
|
||
assert len(new_tape.operations) == 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.
🙌
Co-authored-by: Josh Izaac <josh146@gmail.com>
* tests/transforms/test_optimization/utils.py:
else: | ||
# If the wires are in a different order, gates that are "symmetric" | ||
# over all wires (e.g., CZ), can be cancelled. | ||
if current_gate.is_symmetric_over_all_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.
I've added a bit of extra logic here to handle the additional case where adjacent controlled operations share all controls and a target, but the controls may be in a different order. However this had led to extra branching, and this can probably be streamlined.
…ps://github.com/PennyLaneAI/PennyLane into ch7104-implement-transforms-to-cancel-redundant
>>> print(qml.draw(optimized_qnode)()) | ||
0: ──T──┤ ⟨X⟩ | ||
1: ──Z──┤ |
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.
Agreed, amazing! 🤩
# There are then three possibilities that may lead to inverse cancellation. For a gate U, | ||
# 1. U is self-inverse, and the next gate is also U | ||
# 2. The current gate is U.inv and the next gate is U | ||
# 3. The current gate is U and the next gate is U.inv |
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 this take into account inverse operators that are not next to each other in the queue but can commute through the intermediate gates that seperate them?
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.
A similar question could apply to merging rotators too.
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, it does handle cases where there are gates in between on different qubits (via find_next_gate
). However it doesn't handle actual commutation relations (e.g., T Z T.inv
will not cancel the T
and T.inv
... yet 😄 ).
|
||
Args: | ||
qfunc (function): A quantum function. | ||
atol (float): After fusion of gates, if the fused angle :math:`\theta` is such 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.
Nice catch!
wires_expected = [Wires(0), Wires(1)] | ||
compare_operation_lists(ops, names_expected, wires_expected) | ||
|
||
def test_three_qubits_inverse_after_cnot(self): |
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 this answers my above question about commuting intermediate operations
This looks fantastic @glassnotes ! Some really useful utils functions for compilation too 😁 |
Co-authored-by: anthayes92 <34694788+anthayes92@users.noreply.github.com>
…ps://github.com/PennyLaneAI/PennyLane into ch7104-implement-transforms-to-cancel-redundant
Context: Addition of circuit optimization / compilation tools to PennyLane.
Description of the Change: Creates a subdirectory
transforms/optimization
with new transforms,cancel_inverses
, andmerge_rotations
. TheOperator
class, and relevant single-qubit operations, are augmented with additional attributes to enable this.Benefits: Enables reduction of single-qubit gates in a circuit.
Possible Drawbacks: None
Related GitHub Issues: None