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

[BugFix] - Fix the HilbertSchmidt template #6604

Merged
merged 15 commits into from
Nov 25, 2024

Conversation

PietropaoloFrisoni
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni commented Nov 19, 2024

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]

Copy link
Contributor

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

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

@soranjh soranjh left a comment

Choose a reason for hiding this comment

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

Thanks @PietropaoloFrisoni, looks good to me.

@JerryChen97 JerryChen97 self-requested a review November 20, 2024 21:53
Copy link
Contributor

@JerryChen97 JerryChen97 left a comment

Choose a reason for hiding this comment

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

Looks good for me. Although I'm still wondering if we can have better test explanation, considering the fatality of this bug I only wish this PR merge ASAP
Sry I have new concern below...

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Any way we can test whether or not the template is giving us accurate results? How do we know that the template is doing what it's supposed to be doing?

Given we have the time, I think we should add a test that confirms that we are actually doing what we promise.

@JerryChen97
Copy link
Contributor

JerryChen97 commented Nov 20, 2024

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 JerryChen97 self-requested a review November 20, 2024 22:42
@JerryChen97
Copy link
Contributor

JerryChen97 commented Nov 20, 2024

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

@JerryChen97 JerryChen97 reopened this Nov 20, 2024
@PietropaoloFrisoni
Copy link
Contributor Author

PietropaoloFrisoni commented Nov 21, 2024

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
Copy link
Contributor

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.

@PietropaoloFrisoni PietropaoloFrisoni added the do not merge ⚠️ Do not merge the pull request until this label is removed label Nov 21, 2024
@PietropaoloFrisoni PietropaoloFrisoni removed the do not merge ⚠️ Do not merge the pull request until this label is removed label Nov 22, 2024
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Thanks for tracking down what we really needed to do.

@PietropaoloFrisoni PietropaoloFrisoni enabled auto-merge (squash) November 25, 2024 14:36
@PietropaoloFrisoni PietropaoloFrisoni merged commit 32339ee into master Nov 25, 2024
45 checks passed
@PietropaoloFrisoni PietropaoloFrisoni deleted the Fix_HilbertSchmidt_Template branch November 25, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants