-
Notifications
You must be signed in to change notification settings - Fork 586
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
Controlled Rotation gates added #251
Conversation
Thanks @AroosaIjaz, looks like a useful addition! |
Note that for any new features, new tests should be included. See here: https://github.com/XanaduAI/pennylane/blob/master/.github/PULL_REQUEST_TEMPLATE.md |
Hi @AroosaIjaz, I notice the current test suite is failing for your PR. It looks like the culprit is |
Echoing Nathan, this is a great addition to have in PennyLane, thanks Aroosa! After adding the matrix representation of the gates of def test_controlled_RX_gradient(tol):
"""Test gradient of controlled RX gate"""
dev = qml.device('default.qubit', wires=2)
@qml.qnode(dev)
def circuit(x):
qml.CRX(x, wires=[0, 1])
return qml.expval(qml.PauliZ(0))
a = 0.7664
# get the analytic gradient
gradA = circuit.jacobian([a], method='A')
# get the finite difference gradient
gradF = circuit.jacobian([a], method='F')
# the expected gradient
expected = # put explicit numpy expression in terms of 'a' here
assert np.allclose(gradF, expected, atol=tol, rtol=0)
assert np.allclose(gradA, expected, atol=tol, rtol=0) Feel free to ping me if you have any questions! |
Codecov Report
@@ Coverage Diff @@
## master #251 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 28
Lines ? 1947
Branches ? 0
=======================================
Hits ? 1947
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
Thanks! @co9olguy I have added the matrices in the files you suggested. It passes the current tests. I will try to write tests for these operators too. This is very unfamiliar territory though. @josh146 , Should I also add something like this for controlled gates in def test_x_rotation(self):
#test x rotation is correct
self.logTestName()
# test identity for theta=0
self.assertAllAlmostEqual(Rotx(0), np.identity(2), delta=self.tol)
# test identity for theta=pi/2
expected = np.array([[1, -1j], [-1j, 1]]) / np.sqrt(2)
self.assertAllAlmostEqual(Rotx(np.pi / 2), expected, delta=self.tol)
# test identity for theta=pi
expected = -1j * np.array([[0, 1], [1, 0]])
self.assertAllAlmostEqual(Rotx(np.pi), expected, delta=self.tol) Also what do gradient methods gradA and gradF mean? |
Yes, that would be great if you want to add something like that in test_default_qubit 🙂 Just be aware that test_defualt_qubit is currently written using the older unittest style - we are in the process of transitioning to the new PyTest style of tests (like I commented above). So if you want, you could write the test function using PyTest style.
gradA is the analytic gradient, computed via the parameter shift rule, and gradF is the numerical gradient computed via finite difference. This test checks that both gradient computations agree with the expected gradient result |
@AroosaIjaz Great, thanks! Some additional tests for these gates would definitely be appreciated. You can follow @josh146 's suggestion above, and replace |
is this function available in PennyLane? |
@Eman919191 This PR has not been merged into the master yet so you would not find it in the latest version. I am working on it and it might become available by the end of the month :) In case, you want to use it now, you can directly clone from this PR branch. |
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 @AroosaIjaz! I left a few comments/suggestions.
My major request is to update your tests to have a nonzero gradient case for each gate (more useful for detecting different edge cases than two circuits each with zero gradient).
Co-Authored-By: Nathan Killoran <co9olguy@users.noreply.github.com>
@co9olguy I have added non-zero gradient tests. |
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 @AroosaIjaz, just two more small changes before it's ready!
- can you replace the expected non-zero gradient values with
-np.sin(b)
(easier for maintainability) - you can remove the lines
a=...
since the gradient is zero, we never use a
This is ready for a final review :) |
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 @AroosaIjaz, and sorry I didn't catch before that you were using a
as a parameter to the jacobian 👍
No worries! Thank you for your feedback! :) |
@Eman919191 We sped up the process and added the controlled rotation gates to PennyLane. Thanks a lot for your feedback! Just update your PennyLane software from the github master branch and you should be good to go :) |
Description of the Change:
Added qubit operators CRX, CRY, CRZ, CRot gates in
qubit.py
Fixed an error in the documentation of Rot operator. It says
number of parameters: 1
. it should benumber of parameters: 3
Benefits:
This adds to our set of operators. I might need to use these operators in our current project.