PennyLaneAI / pennylane

PennyLane is a cross-platform Python library for quantum computing, quantum machine learning, and quantum chemistry. Train a quantum computer the same way as a neural network.
https://pennylane.ai
Apache License 2.0
2.38k stars 606 forks source link

[BugFix] - Fix the `HilbertSchmidt` template #6604

Closed PietropaoloFrisoni closed 19 hours ago

PietropaoloFrisoni commented 6 days ago

Context: The HilbertSchmidt and LocalHilbertSchmidt templates do not work correctly since they compute the adjoint instead of the complex conjugate of some operators, as the algorithm requires. For details, see Section 4.1 of this paper

Description of the Change: We compute the complex conjugate instead of the adjoint.

Benefits: Correct result.

Possible Drawbacks: None that I can think of.

Related GitHub Issues: #6586

Related ShortCut Stories: [sc-78349]

github-actions[bot] commented 6 days ago

Hello. You may have forgotten to update the changelog! Please edit doc/releases/changelog-dev.md with:

codecov[bot] commented 5 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.46%. Comparing base (828c1ec) to head (1425561). Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #6604 +/- ## ======================================= Coverage 99.46% 99.46% ======================================= Files 454 454 Lines 42649 42651 +2 ======================================= + Hits 42420 42422 +2 Misses 229 229 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

JerryChen97 commented 5 days ago

I double checked the referenced paper, and realized that there's chance that it's somehow different from the description of the issue that reported this bug: the star sign in the paper is claimed to be the complex conjugate instead of adjoint; this might implies that in the actual implementation, the V* acting on the second half of system is indeed the complex conjugate which should not be reverted in order. Instead, the usage of adjoint might need to be changed to complext conjugate.

Ref: This paper, page 7 the paragraph in front of formula (15); Figure 4 on page 8; and the corresponding paragraph in Appendix A on page 20.

JerryChen97 commented 5 days ago

I double checked the referenced paper, and realized that there's chance that it's somehow different from the description of the issue that reported this bug: the star sign in the paper is claimed to be the complex conjugate instead of adjoint; this might implies that in the actual implementation, the V* acting on the second half of system is indeed the complex conjugate which should not be reverted in order. Instead, the usage of adjoint might need to be changed to complext conjugate.

No more time for deeper reading today but I want to add another comment that similar tricks appear somewhere that people convert the original adjoint operations to composition of complex conjugate plus some other convenient operations

PietropaoloFrisoni commented 5 days ago

I also double-checked in the paper.

I initially took for granted the analysis reported in the description of this issue and on Slack, but now I am no longer sure that this template must be changed.

The circuit that the paper is proposing to implement the Hilbert-Schmidt Test is the one reported in Figure 4 of the paper itself (also shown in the PL documentation).

For this circuit, the probability to obtain the measurement outcome in which all 2n qubits are in the $| 0 \rangle$ state is equal to $\frac{1}{d^2} |\text{Tr}(V^\dagger U)|^2$. The amplitude associated with this probability is $\langle \Phi^+ | U \otimes V^* | \Phi^+ \rangle $, and the paper justifies this step using the following equality (the first passage is apparently known as 'ricochet property' for entangled states in the Q.C. literature):

$$ \langle \Phi^+ | U \otimes V^* | \Phi^+ \rangle = \langle \Phi^+ | U V^\dagger \otimes 1 | \Phi^+ \rangle = \frac{1}{d} \text{Tr}(V^\dagger U). $$

Looking at the left side, I think we should consider the complex conjugate of the trainable unitary V in the circuit, not the adjoint. That is, we should implement the complex conjugate without changing the order. This is equivalent to taking the adjoint as originally implemented:

>>> operations = [qml.RX(0.1, wires=0), qml.RY(0.2, wires=1), qml.CNOT(wires=[0, 1])]
>>> [qml.adjoint(op_v, lazy=False) for op_v in operations]

[RX(-0.1, wires=[0]), RY(-0.2, wires=[1]), CNOT(wires=[0, 1])]

I think we got confused at first because of the presence of qml.adjoint, but perhaps the original intention was simply to apply the complex conjugate.

@albi3ro , @JerryChen97, @soranjh What do you think?

BTW, I just noticed we don't even have a test that verifies the correctness of this template (which is not used in any demo). I totally agree about adding at least a couple of tests to verify that it works as expected.

JerryChen97 commented 5 days ago

I also double-checked in the paper.

I initially took for granted the analysis reported in the description of this issue and on Slack, but now I am no longer sure that this template must be changed.

The circuit that the paper is proposing to implement the Hilbert-Schmidt Test is the one reported in Figure 4 of the paper itself (also shown in the PL documentation).

For this circuit, the probability to obtain the measurement outcome in which all 2n qubits are in the | 0 ⟩ state is equal to 1 d 2 | T r ( V † U ) | 2 . The amplitude associated with this probability is ⟨ Φ + | U ⊗ V ∗ | Φ + ⟩ , and the paper justifies this step using the following equality (the first passage is apparently known as 'ricochet property' for entangled states in the Q.C. literature):

⟨ Φ + | U ⊗ V ∗ | Φ + ⟩ = ⟨ Φ + | U V † ⊗ 1 | Φ + ⟩ = 1 d T r ( V † U ) .

Looking at the left side, I think we should consider the complex conjugate of the trainable unitary V in the circuit, not the adjoint. That is, we should implement the complex conjugate without changing the order. This is equivalent to taking the adjoint as originally implemented:

>>> operations = [qml.RX(0.1, wires=0), qml.RY(0.2, wires=1), qml.CNOT(wires=[0, 1])]
>>> [qml.adjoint(op_v, lazy=False) for op_v in operations]

[RX(-0.1, wires=[0]), RY(-0.2, wires=[1]), CNOT(wires=[0, 1])]

I think we got confused at first because of the presence of qml.adjoint, but perhaps the original intention was simply to apply the complex conjugate.

@albi3ro , @JerryChen97, @soranjh What do you think?

BTW, I just noticed we don't even have a test that verifies the correctness of this template (which is not used in any demo). I totally agree about adding at least a couple of tests to verify that it works as expected.

A test against reliable true value is definitely needed; for a quick validation of this whole workflow I suggest that we can add another test to validate the richochet identity. My gut feeling is we might need to validate whether or not the qml.adjoint equivalently implement the complex conjugate; mathematically only when the unitary is symmetric this works and once asymmetric elements involved (e.g. Ry) this breaks.