-
Notifications
You must be signed in to change notification settings - Fork 586
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 transform for full single-qubit gate fusion #1458
Implement transform for full single-qubit gate fusion #1458
Conversation
…ps://github.com/PennyLaneAI/pennylane into ch7104-implement-transforms-to-cancel-redundant
Co-authored-by: Josh Izaac <josh146@gmail.com>
* tests/transforms/test_optimization/utils.py:
…ps://github.com/PennyLaneAI/PennyLane into ch7104-implement-transforms-to-cancel-redundant
Hello. You may have forgotten to update the changelog!
|
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.
🚤
# Apply the Rot equivalent of the gate, for consistency | ||
Rot(*cumulative_angles, wires=current_gate.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.
Oh interesting --- wouldn't it be better to leave the gates as they are? E.g., CNOT rather than Rot(pi/4, 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.
Because if the circuit is to end up on hardware, something like a CNOT will be easier to implement than Rot(pi/4, 0, 0) (even though they are equivalent)
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 could do either! I was just looking at the docs and realized I'd written "one or more gates gets fused to Rot
", so to match I turned everything to Rot
. But if it's easier to implement just the regular gates, I'll adjust the docstring and revert this back to apply(current_gate)
.
@@ -109,7 +130,7 @@ def qfunc(r1, r2): | |||
next_gate_idx = find_next_gate(current_gate.wires, list_copy[1:]) | |||
|
|||
# Only apply if the cumulative angle is not close to 0 | |||
if not allclose(cumulative_angles, zeros(3), atol=tol): | |||
if not allclose(cumulative_angles, zeros(3), atol=atol): |
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.
Do you want to set rtol=0
here? Otherwise, there remains a relative tolerance check in addition to the absolute tolerance check happening behind the scenes
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.
Alternatively, you could allow the user to pass atol
and rtol
as keyword arguments for full control 😆
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.
Let's set rtol=0
. I'm just thinking intuitively, I would be looking for absolute closeness to 0.
Co-authored-by: Josh Izaac <josh146@gmail.com>
…h7105-implement-transform-for-full-single-qubit
…r excluded gates.
…tps://github.com/PennyLaneAI/PennyLane into ch7105-implement-transform-for-full-single-qubit
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! I think the only thing still missing are tests for the single_qubit_rot_angles
methods added to all the operations. Is there a clean way to easily test that they have the correct angles hardcoded?
This is a small thing that could easily be hiding bugs
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
"""Transforms for optimizing quantum circuits.""" |
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.
Should probably make this slightly more descriptive I suppose
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.
Oops, good catch!
if next_gate_idx is None: | ||
apply(current_gate) | ||
list_copy.pop(0) | ||
continue |
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 like this is missing coverage?
if exclude_gates is not None: | ||
if next_gate.name in exclude_gates: | ||
break |
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.
same here
…tps://github.com/PennyLaneAI/PennyLane into ch7105-implement-transform-for-full-single-qubit
@josh146 thanks for pointing that out, have added a small test for this in the |
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 @glassnotes!
@pytest.mark.parametrize( | ||
"op", | ||
[ | ||
(qml.Hadamard(wires=0)), | ||
(qml.PauliX(wires=0)), | ||
(qml.PauliY(wires=0)), | ||
(qml.PauliZ(wires=0)), | ||
(qml.S(wires=0)), | ||
(qml.T(wires=0)), | ||
(qml.SX(wires=0)), | ||
(qml.RX(0.3, wires=0)), | ||
(qml.RY(0.3, wires=0)), | ||
(qml.RZ(0.3, wires=0)), | ||
(qml.PhaseShift(0.3, wires=0)), | ||
(qml.Rot(0.3, 0.4, 0.5, wires=0)), | ||
], | ||
) | ||
def test_single_qubit_rot_angles(self, op): | ||
"""Tests that the Rot gates yielded by single_qubit_rot_angles | ||
are equivalent to the true operations up to a global phase.""" | ||
angles = op.single_qubit_rot_angles() | ||
obtained_mat = qml.Rot(*angles, wires=0).matrix | ||
|
||
# Check whether the two matrices are each others conjugate transposes | ||
mat_product = qml.math.dot(op.matrix, qml.math.conj(obtained_mat.T)) | ||
mat_product /= mat_product[0, 0] | ||
|
||
assert qml.math.allclose(mat_product, I) |
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 💯
Context: Addition of circuit optimization / compilation tools to PennyLane.
Description of the Change: Adds a new transform that merges arbitrary-length sequences of adjacent single-qubit gates. Adds related tests, and an additional attribute to express single-qubit gates as three rotation angles.
Benefits: Can significantly reduce the length of circuits by squashing any number of single-qubit gates down to one (technically, three, but a single
Rot
).Possible Drawbacks: None
Related GitHub Issues: None