-
Notifications
You must be signed in to change notification settings - Fork 603
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
[OpRefactor] Add custom decomposition context manager to device #1900
Conversation
pennylane/_qubit_device.py
Outdated
# within the context. | ||
def custom_decomp_expand(self, circuit, max_expansion=10): | ||
with self.custom_decomp_context(): | ||
return self.custom_fn(circuit, max_expansion) |
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 with
statement does seem to alter the decomposition; if you do something like print(qml.CNOT.decomposition(wires=[0, 1])
with the new decomposition, it will show up correctly as the custom function, however when the custom expand function is later called, the decomposition is not overridden.
Codecov Report
@@ Coverage Diff @@
## master #1900 +/- ##
=======================================
Coverage 98.83% 98.83%
=======================================
Files 222 222
Lines 16989 17036 +47
=======================================
+ Hits 16791 16838 +47
Misses 198 198
Continue to review full report at Codecov.
|
pennylane/_qubit_device.py
Outdated
|
||
original_decomp_method = obj.decompose | ||
|
||
# This is the method that will override the current .decompose method 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.
The reason why we have to override decompose
instead of decomposition
here is because in the default operator expand
function, this is what is called to perform the decomposition (see here).
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! this is more general as well
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 one disadvantage I can see is that, if a user wanted to define a custom decomposition for a template, this wouldn't work, because many of them don't have a decomposition
, just expand
. But this is maybe not so much the intended use 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.
this will be changed shortly though, right? (@mariaschuld?)
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 prototype seems to work really well 🎉
```pycon | ||
>>> print(qml.draw(decomp_qnode, expansion_strategy="device")(weights)) | ||
0: ──RX(0.4)──────────────────────╭C──RZ(3.14)──RY(1.57)──────────────────────────╭Z──RZ(3.14)──RY(1.57)──┤ ⟨Z⟩ | ||
1: ──RX(0.5)──RZ(3.14)──RY(1.57)──╰Z──RZ(3.14)──RY(1.57)──╭C──────────────────────│───────────────────────┤ | ||
2: ──RX(0.6)──RZ(3.14)──RY(1.57)──────────────────────────╰Z──RZ(3.14)──RY(1.57)──╰C──────────────────────┤ | ||
``` |
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 spent a long time looking at this, but it works out the recursive relationship between the custom CNOT and the custom Hadamard really well! 🎉
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's so wonderful @glassnotes, it feels right to throw this into the device!
pennylane/_qubit_device.py
Outdated
custom_decomp_condition = qml.BooleanFn( | ||
lambda obj: not isinstance(obj, qml.tape.QuantumTape) # Expand templates | ||
and obj.name not in custom_op_names # Expand things that don't have custom decomp | ||
and self.supports_operation(obj.name) # Expand things until supported on 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.
@glassnotes did
custom_decomp_condition = qml.BooleanFn(lambda obj: obj.name not in custom_op_names)
custom_decomp_condition &= self.stopping_condition
not work?
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 the same as the following:
condition = qml.BooleanFn(lambda obj: obj.name not in custom_op_names)
custom_fn = qml.transforms.create_expand_fn(depth=10, stop_at=condition, device=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.
Actually I had a weird amount of trouble with the stopping condition. It seemed like if I did not include that first condition there, it would not expand the templates 😕 Let me give this another go, though.
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.
Ahhh I found the problem I had originally. I was including both obj.name not in custom_op_names
and self.supports_operation(obj.name)
in the stopping condition I was passing to the create_expand_fn
, and the second one was being cancelled out in the &=
with the device criteria. Thanks!
pennylane/_qubit_device.py
Outdated
|
||
original_decomp_method = obj.decompose | ||
|
||
# This is the method that will override the current .decompose method 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.
nice! this is more general as well
|
||
assert len(decomp_ops) == 2 | ||
|
||
assert decomp_ops[0].name == "CRZ" |
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.
Honestly I was kind of surprised this worked out-of-the-box. It seems like the operation being controlled gets decomposed first, and then each gate in the decomposition gets the control applied.
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.
@glassnotes I'm trying to think of any other tests cases, but it sounds like you've covered pretty much all! Only other thing I can think of is a decomposition where:
- template A is decomposed into template B
- gate C is decomposed into gate D
and then decomposing a circuit that has template A?
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 also have to add gradient tests, which I just totally forgot about 😅
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.
sure! Although my gut feeling is that this is one of the parts of the codebase where gradient tests aren't needed, since this PR doesn't introduce any classical processing.
pennylane/_qubit_device.py
Outdated
custom_decomps (Dict[Union(str, qml.operation.Operation), Callable]): Custom | ||
decompositions to be applied by the device at runtime. When specified, the | ||
device will create a custom expand function by combining the regular expansion | ||
criteria with those specified by the custom 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.
Sorry to be a pain, but could we add an example to the docstring? Although the description is accurate, it is so much quicker to see 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.
Actually, it might make more sense to add the example to the device loader, since that is the more user-facing docstring? E.g., here: https://pennylane.readthedocs.io/en/stable/code/api/pennylane.device.html
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, looks like we need to update that docstring anyway to match our latest conventions!
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 meant to ask about this actually, I wasn't sure where it made sense to add this to the docs! Sure, I'll put it on qml.device
page. @josh146 what do you mean by "update that docstring anyway to match our latest conventions"?
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, just following https://pennylane.readthedocs.io/en/latest/development/guide/documentation.html#functions-and-methods, e.g., having the example after the arguments etc.
pennylane/_qubit_device.py
Outdated
@@ -121,7 +125,7 @@ def _scatter(indices, array, new_dimensions): | |||
"Projector", | |||
} | |||
|
|||
def __init__(self, wires=1, shots=None, cache=0, analytic=None): | |||
def __init__(self, wires=1, shots=None, cache=0, analytic=None, custom_decomps=None): |
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.
@mlxd just thought I'd tag you because we are adding stuff to the QubitDevice
class that may have to be ported to Device
one day
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.
Great, thanks for the heads-up @mariaschuld !
pennylane/devices/default_qubit.py
Outdated
def __init__(self, wires, *, shots=None, cache=0, analytic=None, custom_decomps=None): | ||
super().__init__( | ||
wires, shots, cache=cache, analytic=analytic, custom_decomps=custom_decomps | ||
) |
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.
If custom_decomps
is used with external devices (that do not pass it to super
), will nothing happen? Is this a problem?
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.
Ah, this is a very good point. I changed the changelog example to use lightning.qubit
, and got
TypeError: __init__() got an unexpected keyword argument 'custom_decomps'
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.
However, this works fine:
dev = qml.device("lightning.qubit", wires=3)
custom_decomps = {qml.CNOT : custom_cnot, "Hadamard" : custom_hadamard}
custom_decomps = qml.transforms.create_custom_decomp_expand_fn(custom_decomps, dev)
dev.custom_expand(custom_decomps)
@qml.qnode(dev)
def circuit(weights):
qml.BasicEntanglerLayers(weights, wires=[0, 1, 2])
return qml.expval(qml.PauliX(0))
print(qml.draw(circuit, expansion_strategy="device")(weights))
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.
@glassnotes you could create a utility function that combines the calls to create_custom_decomp_expand_fn
and dev.custom_expand
:
@contextlib.contextmanager
def decomposition_context(dev, decomps: dict = None):
expand_fn = create_custom_decomp_expand_fn(decomps, dev)
try:
dev.custom_expand(expand_fn)
yield
finally:
dev.custom_expand_fn = None
Since dev.custom_expand
will globally change the state of the device, I am always concerned about having that be user-facing without wrapping it in a context manager (which provides a context where the change of state is explicit).
This then simplifies the above down to
dev = qml.device("lightning.qubit", wires=3)
@qml.qnode(dev)
def circuit(weights):
qml.BasicEntanglerLayers(weights, wires=[0, 1, 2])
return qml.expval(qml.PauliX(0))
custom_decomps = {qml.CNOT : custom_cnot, "Hadamard" : custom_hadamard}
with decomposition_context(dev, custom_decomps):
print(qml.draw(circuit, expansion_strategy="device")(weights))
which will work on every device without modification. What do you think?
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 must admit I am very much in favour of the latter approach, since it feels more maintainable. Changes to the decomposition API do not require changes to the device API, and vice versa.
It also provides a place in the documentation for documentation and examples.
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.
Ooh this gives me an idea :) In the device loader, you could do
def device(name, *args, **kwargs):
custom_decomp = kwargs.pop("custom_decomp", None)
dev = ... # load the device
if custom_decomp is not None:
custom_decomp_expand_fn = qml.transforms.create_custom_decomp_expand_fn(
custom_decomps, dev
)
dev.custom_expand(custom_decomp_expand_fn)
...
and the subclasses never have to worry :)
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.
with decomposition_context(dev, custom_decomps):
print(qml.draw(circuit, expansion_strategy="device")(weights))
This is like what was implemented in #1872 .
Since dev.custom_expand will globally change the state of the device, I am always concerned about having that be user-facing without wrapping it in a context manager (which provides a context where the change of state is explicit).
In this case though, if the argument is passed upon creation of the device, I would expect the behaviour to be global, and that any time that device is used, the custom decomposition is used.
I do really like the idea of putting this in the device loader though; way more flexible, plus we were going to put the docs there anyways.
@@ -25,6 +27,11 @@ | |||
not_tape, | |||
) | |||
|
|||
# Needed for custom decomposition context manager | |||
from pennylane.transforms.qfunc_transforms import NonQueuingTape |
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 only learnt about this recently :)
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 NonQueuingTape
is something we should definitely move out of the transforms folder! we should put it in tape/
lol
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 feels so natural and intuitive @glassnotes!
I still have to think about this, because my hope here was to eliminate the need for the user to work with the contexts at all, and have everything handled by the device (well, now, the device loader).
Yeah, I guess I just wanted some secret advanced context manager somewhere, just in case I need to help someone with code and an existing device that has already been instantiated/used 😆
the following always works:
expand_fn = qml.transforms.create_decomp_expand_fn(custom_decomps, dev)
dev.custom_expand(expand_fn)
but I was thinking that
with set_decompositions(custom_decomp, dev):
# existing models and executions
could be really nice to whip out in advanced situations/debugging, and a lot safer than calling dev.custom_expand()
, which will cause global state issues
dev = qml.device( | ||
'default.qubit', wires=2, custom_decomps={"CNOT" : ion_trap_cnot} | ||
) |
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 feels very natural and intuitive 🙌
def test_custom_decomp_template_to_template(self): | ||
"""Test that decomposing a template into another template and some | ||
gates yields the correct results.""" |
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>
Ohhhh I see. I misunderstood, I interpreted this as being the way a user would implement this, and not just an alternative method. This is a very easy utility function to add :) |
pennylane/transforms/tape_expand.py
Outdated
if self.num_params == 0: | ||
return fn(self.wires) | ||
return fn(*self.parameters, self.wires) | ||
return 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.
I am momentarily confused, will this line ever be used? Since there is a return function above without an if fork? Maybe this is why the coverage complains?
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.
Whoa!! No, it won't be, removing it has absolutely no effect. I swear, when I checked the coverage yesterday everything was passing 😕
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.
Sorry, just one tiny last thing - the one line that is not covered by tests, would you be able to add a test? I'm particularly asking because I am unsure if the line is ever used?
[sc-7225] |
Context: Operator overhaul
Description of the Change: Adds custom decompositions at the device level by creating a
custom_expand_fn
and context manager. Also adds a context manager to enable custom decompositions to be temporarily used by devices. Subsumes PR #1872.Example: suppose we are running on a trapped-ion machine, and would like to implement a decomposition of the
CNOT
gate intoIsingXX
. We can do so by defining a decomposition (which has the same signature as that of the normal operation), and passing the device a dictionary detailing the custom decompositions.Now, we can see the decomposition get applied by drawing a QNode using the
device
expansion strategy.Benefits: Provides an intuitive means of implementing an arbitrary number of custom decompositions at runtime. There is minimal modification to the code of the device, and no modification of any operator code, since everything is done via context managers.
Possible Drawbacks: Currently, this works by over-riding the
decompose
method of the specified operations within a given context. However this may not work for templates, which implementexpand
.Related GitHub Issues: N/A