Skip to content
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

Fix TrotterProduct differentiability with parameter-shift #6432

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/releases/changelog-dev.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@

<h3>Bug fixes 🐛</h3>

* Fixes incorrect differentiation of `TrotterProduct` when using `diff_method="parameter-shift"`.
[(#6432)](https://github.com/PennyLaneAI/pennylane/pull/6432)

<h3>Contributors ✍️</h3>

This release contains contributions from (in alphabetical order):
Expand Down
1 change: 1 addition & 0 deletions pennylane/ops/op_math/exp.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ class Exp(ScalarSymbolicOp, Operation):

"""

grad_method = None
control_wires = Wires([])
_name = "Exp"

Expand Down
2 changes: 2 additions & 0 deletions pennylane/templates/subroutines/trotter.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ def my_circ(c1, c2, time):
(tensor(0.00961064, requires_grad=True), tensor(-0.12338274, requires_grad=True), tensor(-5.43401259, requires_grad=True))
"""

grad_method = None

@classmethod
def _primitive_bind_call(cls, *args, **kwargs):
# accepts no wires, so bypasses the wire processing.
Expand Down
5 changes: 4 additions & 1 deletion tests/ops/functions/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@
(qml.s_prod(1.1, qml.RX(1.1, 0)), {}),
(qml.prod(qml.PauliX(0), qml.PauliY(1), qml.PauliZ(0)), {}),
(qml.ctrl(qml.RX(1.1, 0), 1), {}),
(qml.exp(qml.PauliX(0), 1.1), {}),
(qml.exp(qml.PauliX(0), 1.1), {"skip_differentiation": True}),
andrijapau marked this conversation as resolved.
Show resolved Hide resolved
# FIXME: Generator of Exp is incorrect when coefficient is imaginary
# (qml.exp(qml.PauliX(0), 2.9j), {}),
(qml.evolve(qml.PauliX(0), -0.5), {}),
(qml.pow(qml.IsingXX(1.1, [0, 1]), 2.5), {}),
(qml.ops.Evolution(qml.PauliX(0), 5.2), {}),
(qml.QutritBasisState([1, 2, 0], wires=[0, 1, 2]), {"skip_differentiation": True}),
Expand Down
38 changes: 36 additions & 2 deletions tests/templates/test_subroutines/test_trotter.py
Original file line number Diff line number Diff line change
Expand Up @@ -447,13 +447,47 @@ def test_copy(self, hamiltonian, time, n, order):
assert op.hyperparameters == new_op.hyperparameters
assert op is not new_op

@pytest.mark.xfail(reason="https://github.com/PennyLaneAI/pennylane/issues/6333", strict=False)
@pytest.mark.parametrize("hamiltonian", test_hamiltonians)
def test_standard_validity(self, hamiltonian):
"""Test standard validity criteria using assert_valid."""
time, n, order = (4.2, 10, 4)
op = qml.TrotterProduct(hamiltonian, time, n=n, order=order)
qml.ops.functions.assert_valid(op)
qml.ops.functions.assert_valid(op, skip_differentiation=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the assert valid test for differentiation fail? I would hope this bug fix would allow that test to pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jaybsoni , I'm slowly re-visiting this PR but the reason was due to Exp also having differentiability issues lol.


@pytest.mark.parametrize("hamiltonian", test_hamiltonians)
def test_differentiation(self, hamiltonian):
"""Tests the differentiation of the TrotterProduct with parameter-shift"""

time, n, order = (4.2, 10, 4)

dev = qml.device("default.qubit")
time = qml.numpy.array(time)
coeffs, _ = hamiltonian.terms()

# FIXME: setting private attribute `_coeffs` as work around
@qml.qnode(dev, diff_method="backprop")
def circ_bp(coeffs, time):
hamiltonian._coeffs = coeffs
qml.TrotterProduct(hamiltonian, time, n, order)
return qml.probs()

@qml.qnode(dev, diff_method="parameter-shift")
def circ_ps(coeffs, time):
hamiltonian._coeffs = coeffs
qml.TrotterProduct(hamiltonian, time, n, order)
return qml.probs()
andrijapau marked this conversation as resolved.
Show resolved Hide resolved

expected_bp = qml.jacobian(circ_bp)(coeffs, time)
ps = qml.jacobian(circ_ps)(coeffs, time)

error_msg = (
"Parameter-shift does not produce the same Jacobian as with backpropagation. "
"This might be a bug, or it might be expected due to the mathematical nature "
"of backpropagation, in which case, this test can be skipped for this operator."
)

for actual, expected in zip(ps, expected_bp):
assert qml.math.allclose(actual, expected), error_msg

# TODO: Remove test when we deprecate ApproxTimeEvolution
@pytest.mark.parametrize("n", (1, 2, 5, 10))
Expand Down