-
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
Allow interface functions to convert QNodes with pre-existing interfaces #707
Conversation
Codecov Report
@@ Coverage Diff @@
## master #707 +/- ##
==========================================
+ Coverage 98.65% 98.66% +0.01%
==========================================
Files 99 103 +4
Lines 6077 6357 +280
==========================================
+ Hits 5995 6272 +277
- Misses 82 85 +3
Continue to review full report at Codecov.
|
@trbromley all tests seem to be passing, except for the Keras tests 🤔 |
Fixed 💪 With |
@josh146 Is there any way to keep QNodes "interface-agnostic", and the interface is just a container/context which contains a "bare" QNode? |
Maybe I'm not fully following, but this is (to some extent) how it is done currently with both the TF and Torch interfaces.
It is difficult to standardize the interface containers much further, since TF requires that the container be The one exception to this is the Autograd interface, where we actually monkeypatch the bare qnode, adding 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 @josh146, looks good to me! Just had some quick questions before approval.
@@ -99,5 +106,6 @@ def gradient_product(g): | |||
|
|||
# define the vector-Jacobian product function for AutogradQNode.evaluate | |||
autograd.extend.defvjp(AutogradQNode.evaluate, AutogradQNode.QNode_vjp, argnums=[1]) | |||
qnode._qnode = qnode # pylint: disable=protected-access |
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.
Could you explain this line? Is this for the case where qnode_interface is 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.
Ah is this part linked to the "possible drawbacks":
While writing tests for to_autograd(), I noticed that it has a side-effect; it mutates the input QNode in addition to returning a new QNode. I attempted to modify to_autograd() to perform a deep copy to ensure this no longer happens. This worked for simple QNodes, but QNode with tensor observables would not copy correctly --- the output would always be the ground state.
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 more because I want to be consistent with the other interfaces -- that a reference to the 'bare' QNode exists at self._qnode
.
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
…ane into fix-interface-conversion
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 !
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Context:
The PennyLane interface functions
to_torch(qnode)
,to_tf(qnode)
, andto_autograd(qnode)
currently all make the assumption that the input QNode either has no attached interface (i.e., it is a 'bare'JacobianQNode
) or that it has an autograd interface.This causes issues if the QNode to be converted has the same interface as the one to be applied (see PL-451, whereby
to_tf(to_tf(qnode))
causes the gradient to be zeroed), or already has a pre-existing interface.Description of the Change:
All interfaces now keep a reference to the original bare QNode under the
_qnode
attribute.The interface functions return the input QNode, without modification, if the input QNode interface matches the function.
If the input QNode has a pre-existing interface, then the interface functions instead return
to_interface(qnode._qnode)
Benefits:
The interface functions contain logic detailing how to handle QNodes with no interfaces and pre-existing interfaces. Higher level abstractions that depend on these functions (such as the
qml.qnn
module) can now use these functions without worrying about the interface of input QNodes.Possible Drawbacks:
While writing tests for
to_autograd()
, I noticed that it has a side-effect; it mutates the input QNode in addition to returning a new QNode. I attempted to modifyto_autograd()
to perform a deep copy to ensure this no longer happens. This worked for simple QNodes, but QNode with tensor observables would not copy correctly --- the output would always be the ground state.Related GitHub Issues: PL-451