-
Notifications
You must be signed in to change notification settings - Fork 585
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 the parameter-shift hessian to the TF interface #1110
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #1110 +/- ##
=======================================
Coverage 98.11% 98.11%
=======================================
Files 144 144
Lines 10775 10794 +19
=======================================
+ Hits 10572 10591 +19
Misses 203 203
Continue to review full report at Codecov.
|
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! Really cool to see Hessian support coming in for the various interfaces 💯
tests/tape/test_jacobian_tape.py
Outdated
@@ -606,7 +606,7 @@ def test_non_differentiable_error(self): | |||
qml.RX(0.543, wires=[0]) | |||
qml.RY(-0.654, wires=[1]) | |||
qml.CNOT(wires=[0, 1]) | |||
qml.probs(wires=[0, 1]) | |||
qml.expval(qml.PauliZ(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.
What was the reason for this change?
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.
@albi3ro was this change you or me?
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 changed it back when we couldn't support calculate the hessian of a vector-valued output. We can now take the hessian of a vector valued function, so I'm changing it back.
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.
@albi3ro in retrospect this was actually a good change, as it increased coverage! I think if you change it back again (sorry 😬 ) codecov will pass and this can be merged in)
|
||
@tf.custom_gradient | ||
def jacobian(p): | ||
def hessian_product(ddy, **tfkwargs): |
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 a problem for this PR, but this nested structure seems a not very maintainable way to provide higher-order gradients. I wonder why this is the standard approach.
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.
Because second derivatives and custom gradients are both not very common in current ML models 😆
[-0.5 * tf.sin(a) * tf.sin(b), 0.5 * tf.cos(a) * tf.cos(b)] | ||
] | ||
] | ||
np.testing.assert_allclose(hess, expected_hess, atol=tol, rtol=0, verbose=True) |
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, I didn't know about np.testing.assert_allclose()
. Is there any advantage over assert np.allclose()
? 🤔
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 curious! 🤔
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.
np.testing.assert_allclose()
provides a better traceback on failure, showing the full arrays it is trying to compare!
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.
Looks nice @albi3ro! 👍
[-0.5 * tf.sin(a) * tf.sin(b), 0.5 * tf.cos(a) * tf.cos(b)] | ||
] | ||
] | ||
np.testing.assert_allclose(hess, expected_hess, atol=tol, rtol=0, verbose=True) |
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 curious! 🤔
Co-authored-by: Theodor <theodor@xanadu.ai>
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.
@albi3ro feel free to merge this one in!
…into hessian_tf
This pull request ties in the parameter shift hessian calculation to the tensorflow interface.
Now you should be able to run: