Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.04k stars 2.33k forks source link

LinCombEstimatorGradient has incorrect sign for imaginary and complex cases. #9112

Closed dlasecki closed 1 year ago

dlasecki commented 1 year ago

Environment

What is happening?

Together with @Cryoris, we noticed that if the IMAG option is set, the resulting gradient has a spurious (undocumented) - sign. Same problem seems to affect the COMPLEX option.

How can we reproduce the issue?

The code

from qiskit import QuantumCircuit
from qiskit.circuit import Parameter
import numpy as np
from qiskit.algorithms.gradients import DerivativeType, LinCombEstimatorGradient
from qiskit.primitives import Estimator
from qiskit.quantum_info import Pauli

estimator = Estimator()
gradient = LinCombEstimatorGradient(estimator, derivative_type=DerivativeType.IMAG)
c = QuantumCircuit(1)
c.rz(Parameter("p"), 0)
grad = gradient.run([c],[Pauli("I")], [[0.0]])
print(grad.result())

prints

EstimatorGradientResult(gradients=[array([-1.])], metadata=[{'parameters': [Parameter(p)], 'derivative_type': <DerivativeType.IMAG: 'imag'>}], options=Options())

What should happen?

For the code example above, the result should be:

EstimatorGradientResult(gradients=[array([1.])], metadata=[{'parameters': [Parameter(p)], 'derivative_type': <DerivativeType.IMAG: 'imag'>}], options=Options())

Any suggestions?

This code fragment

if self._derivative_type == DerivativeType.REAL:
    op1 = SparsePauliOp.from_list([("Z", 1)])
elif self._derivative_type == DerivativeType.IMAG:
    op1 = SparsePauliOp.from_list([("Y", -1)])
elif self._derivative_type == DerivativeType.COMPLEX:
    op1 = SparsePauliOp.from_list([("Z", 1)])
    op2 = SparsePauliOp.from_list([("Y", -1)])

should be replaced with

if self._derivative_type == DerivativeType.REAL:
    op1 = SparsePauliOp.from_list([("Z", 1)])
elif self._derivative_type == DerivativeType.IMAG:
    op1 = SparsePauliOp.from_list([("Y", 1)])
elif self._derivative_type == DerivativeType.COMPLEX:
    op1 = SparsePauliOp.from_list([("Z", 1)])
    op2 = SparsePauliOp.from_list([("Y", 1)])

It also seems that unit tests for IMAG and COMPLEX options are missing.

woodsp-ibm commented 1 year ago

@a-matsuo

a-matsuo commented 1 year ago

Are you sure it's a bug? 🤔 I thought the - sign is necessary. https://github.com/Qiskit/qiskit-terra/blob/794db4792e97adca4a78eb4f3411c04e50665df7/qiskit/opflow/gradients/circuit_gradients/lin_comb.py#L618

dlasecki commented 1 year ago

Are you sure it's a bug? 🤔 I thought the - sign is necessary.

https://github.com/Qiskit/qiskit-terra/blob/794db4792e97adca4a78eb4f3411c04e50665df7/qiskit/opflow/gradients/circuit_gradients/lin_comb.py#L618

Yes, it was like this in opflow but we discussed with @Cryoris that this is an undesired behavior.

Cryoris commented 1 year ago

For context, the docstring says that

2 (dω⟨ψ(ω)|)O(θ)|ψ(ω)〉

is computed. If we take the example that the observable O is just the identity and the state is prepared by RZ(ω)|0〉, then

2 (dω⟨ψ(ω)|)O(θ)|ψ(ω)〉
= 2 (-i/2 RZ(ω) Z |0〉)^\dagger  RZ(ω)|0〉
= i ⟨0|Z RZ(-ω) RZ(ω)|0〉
= i

but the current gradients returns -i.

a-matsuo commented 1 year ago

I talked with @Cryoris. If you differentiate the gate on the the left hand side, then you take Im, it will be like that. How ever if you differentiate the gate on the right hand side, the sign will be a minus (It’s because imaginary parts will be cancelled out in the end, so which side we use matters) image https://pennylane.ai/qml/demos/tutorial_adjoint_diff.html

We couldn't find the exact definition of imaginary gradients (not qiskit's docstring. It might be using the wrong definition), so it’s up to our definition. If we change this, we need to change this line in QFI. Because, I suppose the return value is the right hand side gradient. https://github.com/Qiskit/qiskit-terra/blob/ee0b0368e72913cddf1c80ed95bc55e174c65046/qiskit/algorithms/gradients/lin_comb_qfi.py#L217

An alternative way is to change the docstring in qiskit. Then, we don’t need to change the code, and we don’t need to change the QFI.

@dlasecki What do you think?

Cryoris commented 1 year ago

After discussing with @dlasecki @Zoufalc and @a-matsuo it seems that the conclusion is to change the documentation and not the code, because

  1. differentiating left or right is equivalent; we just have to fix a definition
  2. this way, we don't have to break the code behavior and just update the docs
dlasecki commented 1 year ago

After discussing with @dlasecki @Zoufalc and @a-matsuo it seems that the conclusion is to change the documentation and not the code, because

  1. differentiating left or right is equivalent; we just have to fix a definition
  2. this way, we don't have to break the code behavior and just update the docs

Yes, sounds good to me. I will adapt the PR that I opened already.