-
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 support for qml.specs to the beta QNode #1739
Conversation
Hello. You may have forgotten to update the changelog!
|
[sc-9747] |
Codecov Report
@@ Coverage Diff @@
## master #1739 +/- ##
==========================================
+ Coverage 96.55% 99.22% +2.66%
==========================================
Files 207 207
Lines 15552 15573 +21
==========================================
+ Hits 15017 15452 +435
+ Misses 535 121 -414
Continue to review full report at Codecov.
|
@albi3ro I've updated the test file to retain the original tests and the new tests, ensuring |
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 some thoughts and questions for now.
pennylane/transforms/specs.py
Outdated
info["execution_options"] = qnode.execute_kwargs | ||
info["interface"] = qnode.interface | ||
|
||
if callable(qnode.gradient_fn): |
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 else would it be? A string? 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.
Yep, it can currently take three types:
- Callable: one of
qml.gradients.gradient_transform
(could be changed toisinstance(qnode.gradient_fn, gradient_transform)
to be stricter) None
: no gradientsstr
: to indicate"device"
"The QNode specifications can only be calculated after its quantum tape has been constructed." | ||
) | ||
|
||
info = qnode.qtape.specs.copy() |
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.
Will we be getting rid of the qnode.specs
property?
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; the idea is to remove all properties/methods that depend on the state of construction, and replace them with functions that act on the QNode. This also includes qnode.draw()
and qnode.metric_tensor()
.
pennylane/transforms/specs.py
Outdated
# In the case of a broad exception, we don't want the `qml.specs` transform | ||
# to fail. Instead, we simply indicate that the number of gradient executions | ||
# is not supported. |
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.
While this might be helpful while the new QNode is still under development, in the long run, we should know exactly the types of situations that would cause this to fail and check for those instead. If something's going wrong internally, we want to know.
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, this was inserted to solve an error I was getting while running the tests! In particular, this one:
@qml.qnode(dev, diff_method=diff_method)
def circuit():
return qml.state()
Of the three diff methods tested, only backprop
supports state differentiation; both adjoint
and parameter-shift
will raise an error.
-
Previously,
qml.specs
would use theOperator.grad_recipe
to determine the number of shifts required, independent of the circuit measurement, resulting in an incorrect value fornum_gradient_executions
. -
In this PR,
qml.specs
queries directly the gradient transform to determine the number of shifts. This results in more accurate results, however it means that callingqml.specs(circuit)
on the QNode above will raise an error, sinceqml.gradients.param_shift
andadjoint
raise an exception if the input circuit ends withqml.state
.
Overall, I think this is more informative; checking the number of parameter-shift executions on this QNode will show NotSupported
, rather than a numeric value.
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 we could do, though, is somehow 'catch' the exception message, and store this in the dictionary?
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 pushed an update that does the following 🙂
dev = qml.device("default.qubit", wires=2)
@qml.beta.qnode(dev, diff_method=qml.gradients.param_shift)
def circuit(x):
qml.RX(x, wires=0)
return qml.state()
>>> specs = qml.specs(circuit)(0.56)
>>> specs["num_gradient_executions"]
NotSupported: Computing the gradient of circuits that return the state is not supported.
pennylane/transforms/specs.py
Outdated
info["gradient_fn"] = inspect.getmodule(qnode.gradient_fn).__name__ | ||
|
||
try: | ||
if isinstance(qnode.gradient_fn, qml.gradients.gradient_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.
This seems to already be inside an if callable(qnode.gradient_fn)
block. Is there a reason we make the stricter check again? Or can we defer this check to the outer if statement?
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, let me remove the inner 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.
A few questions about when:
A) someone passes in a gradient transform that's built into pennylane, like qml.gradients.param_shift
B) someone passes in their own custom gradient transform
I think those cases need to be improved and tested.
Co-authored-by: Christina Lee <christina@xanadu.ai>
Thanks Christina, both valid points! I've pushed a commit that takes this into account, and now shows the absolute import path for gradient transforms, even when a custom 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.
Thanks for the improvements!
Context: Previous,
qml.specs
would queryQNode.specs
. This property does not exist on the new QNode.Description of the Change:
qml.specs
transform is modified to directly compute the specifications ofqml.beta.QNode
objects.if
-else
block used to temporarily set the QNode expansion depth is changed totry
-finally
, to ensure it is safe in case there is an exception.Benefits:
Possible Drawbacks: n/a
Related GitHub Issues: n/a