-
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 transform to insert operations into circuits #1795
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1795 +/- ##
=======================================
Coverage 98.92% 98.92%
=======================================
Files 208 209 +1
Lines 15655 15708 +53
=======================================
+ Hits 15486 15539 +53
Misses 169 169
Continue to review full report at Codecov.
|
pennylane/transforms/noise.py
Outdated
noisy_op_args: Union[tuple, float], | ||
position: str = "all", | ||
) -> QuantumTape: | ||
r"""Add noisy operations to an input 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.
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.
Hey Tom, this is all WIP as we understand the best workflow for transforms, but I'm wondering if it makes sense to simply add the @qml.qfunc_transform
here, and just call this add_noise
:
@qml.qfunc_transform
def add_noise(tape, noisy_op, ...):
return new_tape
Then, both approaches are accessible:
@qml.qnode(dev)
@add_noise(qml.AmplitudeDamping, 0.2, position="end")
def f(w, x, y, z):
qml.RX(w, wires=0)
qml.RY(x, wires=1)
qml.CNOT(wires=[0, 1])
qml.RY(y, wires=0)
qml.RX(z, wires=1)
return qml.expval(qml.PauliZ(0) @ qml.PauliZ(1))
>>> print(qml.draw(f)(0.9, 0.4, 0.5, 0.6))
0: ──RX(0.9)──╭C──RY(0.5)──AmplitudeDamping(0.2)──╭┤ ⟨Z ⊗ Z⟩
1: ──RY(0.4)──╰X──RX(0.6)──AmplitudeDamping(0.2)──╰┤ ⟨Z ⊗ Z⟩
The tape transform is accessible via transform.tape_fn
:
with qml.tape.QuantumTape() as tape:
qml.RX(0.9, wires=0)
qml.RY(0.4, wires=1)
qml.CNOT(wires=[0, 1])
qml.RY(0.5, wires=0)
qml.RX(0.6, wires=1)
qml.expval(qml.PauliZ(0) @ qml.PauliZ(1))
new_tape = add_noise.tape_fn(tape, qml.AmplitudeDamping, 0.2, position="end")
>>> print(new_tape.draw())
0: ──RX(0.9)──╭C──RY(0.5)──AmplitudeDamping(0.2)──╭┤ ⟨Z ⊗ Z⟩
1: ──RY(0.4)──╰X──RX(0.6)──AmplitudeDamping(0.2)──╰┤ ⟨Z ⊗ 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 solves the docstring issue, and avoids the duplication.
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, with a two-line change to qfunc_transform
, we can support single-dispatch-style behaviour, where the transform acts correctly depending on the argument type:
new_qnode = add_noise(qml.AmplitudeDamping, 0.2, position="end")(qnode)
new_tape = add_noise(qml.AmplitudeDamping, 0.2, position="end")(tape)
This is supported by the following diff:
diff --git a/pennylane/transforms/qfunc_transforms.py b/pennylane/transforms/qfunc_transforms.py
index b631f39c..6394c0d1 100644
--- a/pennylane/transforms/qfunc_transforms.py
+++ b/pennylane/transforms/qfunc_transforms.py
@@ -172,6 +172,9 @@ class single_tape_transform:
def _create_qfunc_internal_wrapper(fn, tape_transform, transform_args, transform_kwargs):
"""Convenience function to create the internal wrapper function
generated by the qfunc_transform decorator"""
+ if isinstance(fn, qml.tape.QuantumTape):
+ return tape_transform(fn, *transform_args, **transform_kwargs)
+
if not callable(fn):
raise ValueError(
f"The qfunc to transform, {fn}, does not appear "
if you would like to add 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.
Thanks Josh for the feedback! We now have add_noise
which primarily acts as a qfunc/QNode transform, with the tape transform accessed via add_noise.tape_fn
. I briefly tried the single dispatch approach suggested in your last comment but had some trouble, so thought it best to leave out for now.
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>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
* Move device transform API into the @qfunc_transform decorator * linting * add to changelog * support drawing expansions * typo * Apply suggestions from code review Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com> * Update pennylane/transforms/draw.py Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com> Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.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.
Just left a couple minor suggestions, looks awesome @trbromley ! 🎉
the internal circuit such that all circuit operations are supported by the gradient | ||
method. | ||
|
||
- ``device``: The QNode will attempt to decompose the internal circuit |
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.
Ohhh this will be really really convenient for future device-specific compilation transforms like allocation and routing, it will easily allow you to see the before and after!
|
||
@qfunc_transform | ||
def insert( | ||
circuit: Union[callable, QuantumTape, Device], |
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.
Nothing actionable for this suggestion, but I did not notice there were type hints during my first review. I think this is the first time I've seen them in PL; is it something we'd like to add to more stuff, or is it just for convenience here because this transform can be called in a lot of different ways?
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.
The official policy so far has been that type hinting is optional and up to the developer; feel free to add it if you would like, but it is not essential.
Perhaps something we will evaluate down the line, especially if we decide to start using mypy on the CI.
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 really like type hinting because my IDE lets me know if I'm returning/passing arguments different to the type hints. Though the rendering in the docstrings doesn't look great.
acting on a single qubit, to be inserted into the circuit | ||
op_args (tuple or float): the arguments fed to the operation, either as a tuple or a single | ||
float | ||
position (str): Specification of where to add the operation. Should be one of: ``"all"`` to |
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.
Makes sense! 💯
qml.RX(0.6, wires=1) | ||
qml.expval(qml.PauliZ(0) @ qml.PauliZ(1)) | ||
|
||
assert all(o1.name == o2.name for o1, o2 in zip(tape.operations, tape_exp.operations)) |
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 no worries, and agree, a broader equivalence check mechanism would be really handy!
pennylane/transforms/insert_ops.py
Outdated
add the operation after all gates; ``"start"`` to add the operation to all wires | ||
at the start of the circuit; ``"end"`` to add the operation to all wires at 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.
Perhaps mention here the interaction with state prep operations and 'start'
option. (Or maybe better in usage details?)
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 point, made that change: 96cb3d0
Co-authored-by: Josh Izaac <josh146@gmail.com> Co-authored-by: Olivia Di Matteo <2068515+glassnotes@users.noreply.github.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.
Thanks @josh146 @glassnotes for the reviews!
pennylane/transforms/insert_ops.py
Outdated
add the operation after all gates; ``"start"`` to add the operation to all wires | ||
at the start of the circuit; ``"end"`` to add the operation to all wires at 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.
Good point, made that change: 96cb3d0
Context:
Add
insert
andinsert_in_dev
, which can be used to insert operations into fixed points in a circuit (e.g. for the purposes of adding noise):For devices:
Description of the Change:
Add a new
insert_ops
submodule to thetransforms
module containing the above functions, as well as tests and updating docs.The
insert_in_dev
prototypes a device transform by overwriting the device'sexpand_fn
.Benefits:
Users can easily add prototypical noise channels to a quantum circuit.
Possible Drawbacks:
execute
UIRelated:
An overrotation type of noise (which preserves purity) was discussed here.