-
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
specs transform #1245
specs transform #1245
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #1245 +/- ##
=======================================
Coverage 98.20% 98.21%
=======================================
Files 159 160 +1
Lines 11838 11888 +50
=======================================
+ Hits 11626 11676 +50
Misses 212 212
Continue to review full report at Codecov.
|
qnode.construct(args, kwargs) | ||
tape = qnode.qtape |
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.
qnode.construct(args, kwargs) | |
tape = qnode.qtape | |
tape = qnode.construct(args, kwargs) |
You can skip a step 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.
yeah that doesn't work.
"""Code for resource estimation""" | ||
|
||
|
||
def resource_estimation(qnode, expand_depth=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.
Hmm... not sure how I feel about expand_depth
being here. But that leads into a whole other conversation around how tapes get expanded on the device, and that's probably outside of the design scope here....
qnode (qml.QNode): a PL Qnode | ||
|
||
Returns: | ||
function: a function of the same parameters as the qnode |
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.
function: a function of the same parameters as the qnode | |
A function that has same inputs as the given qnode and returns a resource estimation dictionary. |
Yeah this looks pretty good! Now all we need is a function |
(Just a fly-by comment): It might be worth absorbing the existing |
pennylane/transforms/specs.py
Outdated
executed on a device. Expansion occurs when an operation or measurement is not | ||
supported, and results in a gate decomposition. If any operations in the decomposition | ||
remain unsupported by the device, another expansion occurs. Defaults to | ||
``qnode.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.
Just to understand, why do we give access to exactly that one? For example, I would find it much more useful to give user access to the stopping criterion for expansion, if anything...
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'm just mirroring how QNode's 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.
I also wondered 🤔. If we have to expand to depth N
for the device to execute, what would happen if max_expansion < N
? Wouldn't it just not work on the device? Although I guess you can still access the specs?
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.
E.g.,
dev = qml.device('default.qubit', wires=2)
@qml.qnode(dev)
def circuit():
qml.templates.StronglyEntanglingLayers(np.random.random((3, 2, 3)), wires=range(2))
return qml.probs(wires=(0,1))
qml.specs(circuit, max_expansion=0)()
tests/tape/test_qnode.py
Outdated
|
||
assert info["num_device_wires"] == 4 | ||
assert info["device_name"] == "default.qubit" | ||
assert info["diff_method"] == "adjoint" |
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 it would be nice to add tests for:
- a qnode without gates
- a qnode where the tape changes with the arguments
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.
But nice to have the tests in different diff methods!
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.
Some slight variation across diff methods, like adding parameter shift executions or deleting "num_trainbable_params"
.
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.
Would also be nice to have a case for the behaviour of having multiple measurements in the return
statement of the qfunc.
tests/tape/test_tape.py
Outdated
assert tape.specs["num_trainable_params"] == 0 | ||
assert tape.specs["depth"] == 0 | ||
|
||
assert len(tape.specs) == 7 |
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 nice, ignore my comment above then!
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.
As I found out, empty circuits have their own peculiarties, so I'm adding tests for empty qnodes.
.github/CHANGELOG.md
Outdated
'by_name': defaultdict(int, {'RX': 1, 'Toffoli': 1, 'CRY': 1, 'Rot': 1}), | ||
'total_operations': 4, | ||
'total_observables': 2, | ||
'num_tape_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.
I would not talk about tapes, since this is not user facing at all. Can we say num_used_wires
then?
.github/CHANGELOG.md
Outdated
'total_operations': 4, | ||
'total_observables': 2, | ||
'num_tape_wires': 3, | ||
'depth': 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.
What about "circuit_depth"?
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 did not to the deepest review, but went through everything. Giving approval because I think as it is it will improve the code base, and there was nothing wrong I could see (plus I will be on vacation). My only suggestions (non-blocking, but strongly recommended) are namings.
Very nice addition @albi3ro , and it uncovered some unelegant parts of the deepest qnode code base.
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 @albi3ro! So far I have just looked at the changelog and tried out this new feature. It's very cool and will be fun to use! I left a few questions that would be good to understand.
.github/CHANGELOG.md
Outdated
'depth': 3, | ||
'num_trainable_params': 4, | ||
'num_parameter_shift_executions': 7, | ||
'num_device_wires': 4, |
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.
Side note: it'd be great to finally get rid of device wires for simulators, and only use as many wires as there are in the tape!
.github/CHANGELOG.md
Outdated
'num_tape_wires': 3, | ||
'depth': 3, | ||
'num_trainable_params': 4, | ||
'num_parameter_shift_executions': 7, |
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 include the forward pass execution? E.g., if you set requires_grad=False
for x
, this number is 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.
Also, how is this number 7
? Since we have 4
params shouldn't it be 9
?
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.
Would this make sense as num_device_executions
?
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 @albi3ro!
pennylane/qnode.py
Outdated
@@ -720,6 +720,63 @@ def circuit(): | |||
charset=charset, wire_order=wire_order, show_all_wires=show_all_wires | |||
) | |||
|
|||
@property | |||
def specs(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.
pennylane/qnode.py
Outdated
# tape's do not accurately track parameters for backprop | ||
# TODO: calculate number of trainable parameters in backprop | ||
# find better syntax for determining if backprop | ||
if info["diff_method"] == "backprop": | ||
del info["num_trainable_params"] |
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, probably answers my question above.
pennylane/tape/qubit_param_shift.py
Outdated
def specs(self): | ||
|
||
if "grad_method" not in self._par_info[0]: | ||
self._update_gradient_info() |
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 this for if someone asks for specs before actually having evaluated the gradient?
pennylane/tape/tape.py
Outdated
else: | ||
self._resources[op.name] += 1 | ||
warnings.warn( | ||
"``tape.get_resources`` will be deprecated after v0.16 " |
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 we want to say when it will be removed?
E.g.
"tape.get_resources
is now deprecated and will be removed in v0.17"
pennylane/transforms/specs.py
Outdated
|
||
|
||
def specs(qnode, max_expansion=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.
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.
Indeed, and worth including that a QNode construction is required to obtain data here.
info = super().specs | ||
info["num_parameter_shift_executions"] = num_executions | ||
|
||
return info |
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 just realised specs
doesn't work so well with circuits that include templates:
import pennylane as qml
from pennylane import numpy as np
x = np.array([0.1, 0.2])
with qml.tape.QubitParamShiftTape() as tape:
qml.templates.StronglyEntanglingLayers(np.random.random((3, 2, 3)), wires=range(2))
tape.specs
Gives
{'by_size': defaultdict(int, {2: 1}),
'by_name': defaultdict(int, {'StronglyEntanglingLayers': 1}),
'total_operations': 1,
'total_observables': 0,
'num_tape_wires': 2,
'depth': 1,
'num_trainable_params': 1,
'num_parameter_shift_executions': 1}
Though in practice, there are many trainable params and there will be many param shift executions 🤔.
There may not be an easy fix for this though. And also things work nicely on the QNode level as expansion will have happened.
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 think this is something that needs fixing for this PR? Most users are not going to working with tapes, and there are multiple shortcomings to a tape with an unexpanded template.
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 agree, maybe something we can think about as we improve the Operation
class. (I believe this subtlety is due to moving templates to operations, since they'd be automatically expanded before?)
pennylane/transforms/specs.py
Outdated
executed on a device. Expansion occurs when an operation or measurement is not | ||
supported, and results in a gate decomposition. If any operations in the decomposition | ||
remain unsupported by the device, another expansion occurs. Defaults to | ||
``qnode.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.
I also wondered 🤔. If we have to expand to depth N
for the device to execute, what would happen if max_expansion < N
? Wouldn't it just not work on the device? Although I guess you can still access the specs?
pennylane/transforms/specs.py
Outdated
executed on a device. Expansion occurs when an operation or measurement is not | ||
supported, and results in a gate decomposition. If any operations in the decomposition | ||
remain unsupported by the device, another expansion occurs. Defaults to | ||
``qnode.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.
E.g.,
dev = qml.device('default.qubit', wires=2)
@qml.qnode(dev)
def circuit():
qml.templates.StronglyEntanglingLayers(np.random.random((3, 2, 3)), wires=range(2))
return qml.probs(wires=(0,1))
qml.specs(circuit, max_expansion=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.
This is looking great! 😊 Will be very useful. My comments are mostly for consideration.
pennylane/tape/tape.py
Outdated
|
||
for op in self.operations: | ||
# don't use op.num_wires to allow for flexible gate classes like hermitian | ||
self._specs["by_size"][len(op.wires)] += 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.
Perhaps this was decided on previously, but the names "by_size"
and "by_name"
do not seem completely descriptive, how about pretending "ops_"
to them? e.g., "ops_by_size"
, etc.
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.
Due to Maria's suggestion, we are now using "gate_sizes"
and "gate_types"
pennylane/tape/tape.py
Outdated
|
||
self._specs["total_operations"] = len(self.operations) | ||
self._specs["total_observables"] = len(self.observables) | ||
self._specs["num_tape_wires"] = self.num_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.
How about the "num_all_wires"
or simply "all_wires"
here? "num_tape_wires"
assumes knowledge about having quantum tapes represent quantum circuits. When querying a QNode, a user might not possess the knowledge that there's an underlying quantum tape in a QNode and might become confused.
tests/tape/test_qnode.py
Outdated
assert info["num_tape_wires"] == 3 | ||
assert info["depth"] == 3 | ||
assert info["num_trainable_params"] == 4 | ||
assert info["num_parameter_shift_executions"] == 7 |
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.
Maybe these three tests could be parametrized, as this seems to be the only line that is different. This one could be checked with an if
statement.
pennylane/tape/qubit_param_shift.py
Outdated
# Loop over all variables | ||
for _, info in self._par_info.items(): | ||
|
||
if info["grad_method"] == "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.
How about finite diff and CV circuits? Maybe not too important for now, but might be worth considering later.
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 thought about it a little and will probably try to add them in later.
tests/tape/test_qnode.py
Outdated
|
||
assert info["num_device_wires"] == 4 | ||
assert info["device_name"] == "default.qubit" | ||
assert info["diff_method"] == "adjoint" |
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.
Would also be nice to have a case for the behaviour of having multiple measurements in the return
statement of the qfunc.
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com> Co-authored-by: Maria Schuld <mariaschuld@gmail.com> Co-authored-by: antalszava <antalszava@gmail.com>
…pennylane into resource_estimation
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 @albi3ro! Just left some suggestions, otherwise looks awesome!
|
||
info = super().specs | ||
|
||
if any(m.return_type is qml.operation.State for m in self.measurements): |
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 to check, this also works for density matrix too right? 🤔
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.
Yep. It was a state thing.
for _, grad_info in self._par_info.items(): | ||
|
||
# if this variable uses parameter-shift | ||
if grad_info["grad_method"] == "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.
Do we ever have finite-diff gates in the qubit setting? 🤔
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.
Adding finite-diff is on the todo list.
info = super().specs | ||
info["num_parameter_shift_executions"] = num_executions | ||
|
||
return info |
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 agree, maybe something we can think about as we improve the Operation
class. (I believe this subtlety is due to moving templates to operations, since they'd be automatically expanded before?)
pennylane/transforms/specs.py
Outdated
* ``"total_operations"`` | ||
* ``"total_observables"`` | ||
* ``"by_size"``: dictionary mapping gate number of wires to number of occurances | ||
* ``"by_name"``: dictionary mapping gate types to number of occurances | ||
* ``"num_tape_wires"``: number of wires used by the circuit | ||
* ``"num_wires"``: number of wires in device | ||
* ``"depth"``: longest path in directed acyclic graph representation | ||
* ``"dev_short_name"``: name of QNode device | ||
* ``"diff_method"`` | ||
|
||
Potential Additional Information: | ||
* ``"num_trainable_params"``: number of individual scalars that are trainable | ||
* ``"num_parameter_shift_executions"``: number of times circuit will execute when |
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 to check, is this up to date? Maybe the diagonalizing gates one.
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.
Not up to date. Thanks for catching that! Tiny little changes having to get propagated to a million places.
assert info["depth"] == 1 | ||
assert info["num_device_wires"] == 5 | ||
assert info["device_name"] == "default.qubit.autograd" | ||
assert info["diff_method"] == "backprop" |
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.
My only worry would be if there's any more edge cases, but the tests pass and coverage is 100% so that should be good!
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
This PR introduces a "transform" that calculates circuit resources. The function
qml.transforms.resource_estimation
takes a qnode, and returns a function. When that function is called with circuit arguments, it returns a dictionary of information.The current values are
dev_short_name
num_wires
: the number of wires on the devicenum_gates
: total number of gatesnum_ops_by_size
: a dictionary that gives the number of operations of a certain size. For example, the above circuit has three single qubit gates and three two-qubit gates.The
qml.transforms.resource_estimation
function can take in a keywordexpand_depth
, but it looks likeqnode.construct
expanded everything anyway.In the future, I would like to add time scaling for applicable devices given relevant circuit structure information.