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

Quantum shannon decomposition fails for QFT with non-unitary matrix error #6666

Closed
doublehoon opened this issue Jul 16, 2024 · 16 comments · Fixed by #6756
Closed

Quantum shannon decomposition fails for QFT with non-unitary matrix error #6666

doublehoon opened this issue Jul 16, 2024 · 16 comments · Fixed by #6756
Labels
kind/bug-report Something doesn't seem to work. triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add

Comments

@doublehoon
Copy link

Description of the issue

I encountered an issue when attempting to decompose the unitary matrix of QFT(4) using the built-in quantum_shannon_decomposition function in Cirq.

According to the documentation, the quantum_shannon_decomposition function is supposed to decompose the unitary matrix into a sequence of CX, YPow, ZPow, and CNOT gates. However, it seems that the result of the decomposition contains a non-unitary matrix.

How to reproduce the issue

import cirq

qft4_qubits = cirq.NamedQubit.range(4, prefix="q")
qft4_op = cirq.qft(*qft4_qubits)
qft4_qc = cirq.Circuit(qft4_op)

qft4_utry = cirq.unitary(qft4_qc)

print(cirq.is_unitary(qft4_utry))   # True

qsd_qft4_qubits = cirq.NamedQubit.range(4, prefix="q")
qsd_qft4_op = cirq.quantum_shannon_decomposition(qsd_qft4_qubits, qft4_utry)
qsd_qft4_qc = cirq.Circuit(qsd_qft4_op)

qsd_qft4_utry = cirq.unitary(qsd_qft4_qc)

print(cirq.allclose_up_to_global_phase(qft4_utry, qsd_qft4_utry))
True

Traceback (most recent call last):
  File "/tmp/qsd_bug_cirq.py", line 11, in <module>
    qsd_qft4_qc = cirq.Circuit(qsd_qft4_op)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/anaconda3/envs/qcs/lib/python3.11/site-packages/cirq/circuits/circuit.py", line 1771, in __init__
    flattened_contents = tuple(ops.flatten_to_ops_or_moments(contents))
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/anaconda3/envs/qcs/lib/python3.11/site-packages/cirq/ops/op_tree.py", line 134, in flatten_to_ops_or_moments
    yield from flatten_to_ops_or_moments(subtree)
  File "/anaconda3/envs/qcs/lib/python3.11/site-packages/cirq/ops/op_tree.py", line 133, in flatten_to_ops_or_moments
    for subtree in root:
  File "/anaconda3/envs/qcs/lib/python3.11/site-packages/cirq/transformers/analytical_decompositions/quantum_shannon_decomposition.py", line 89, in quantum_shannon_decomposition
    yield from _msb_demuxer(qubits, v1, v2)
  File "/anaconda3/envs/qcs/lib/python3.11/site-packages/cirq/transformers/analytical_decompositions/quantum_shannon_decomposition.py", line 158, in _msb_demuxer
    yield from quantum_shannon_decomposition(demux_qubits[1:], W)
  File "/anaconda3/envs/qcs/lib/python3.11/site-packages/cirq/transformers/analytical_decompositions/quantum_shannon_decomposition.py", line 66, in quantum_shannon_decomposition
    raise ValueError(
ValueError: Expected input matrix u to be unitary,                 but it fails cirq.is_unitary check

Cirq version
1.4.1

@doublehoon doublehoon added the kind/bug-report Something doesn't seem to work. label Jul 16, 2024
@doublehoon doublehoon changed the title Quantum Shannon Decomposition Fails for QFT with Non-Unitary Matrix Error Quantum shannon decomposition fails for QFT with non-unitary matrix error Jul 16, 2024
@NoureldinYosri NoureldinYosri added the triage/discuss Needs decision / discussion, bring these up during Cirq Cynque label Jul 16, 2024
@dstrain115 dstrain115 added triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Jul 17, 2024
@NoureldinYosri
Copy link
Collaborator

@uzzzzzzz can you take a look? since you implemented this #6020

@pavoljuhas
Copy link
Collaborator

csynque meeting - we need to confirm this fails due to significant difference rather than round-off errors.

@NoureldinYosri
Copy link
Collaborator

related to #6725

@pavoljuhas it seems that quantum_shannon_decomposition is indeed buggy

@ACE07-Sev
Copy link

@NoureldinYosri I would say this is a good opportunity to rewrite how cirq does QSD for two reasons:

  1. It doesn't work as is.
  2. Compared to Qiskit's QSD, it's 14 percent more expensive in terms of depth. This is an extremely important compilation which would directly dictate the size of problems you can meaningfully map to actual hardware.

It would be great if you can just replicate what Qiskit has for your QSD. I'd be more than happy to help where I can.

Here's a reference for QSD through qiskit vs through cirq (let's assume it's fixed):
image

Cirq's depth: (7, 49, 235, 1027, 4291, 17539, 70915, 285185)
Qiskit's depth: (7, 41, 197, 869, 3653, 14981, 60677, 244229)

@ACE07-Sev
Copy link

I think it would be worthwhile to add two qubit decomposition function as I mentioned before. Looking at Qiskit's version VS Cirq's version:
image
image
There's a considerable difference. Besides that, the rest is the same from what I can infer.

@NoureldinYosri
Copy link
Collaborator

NoureldinYosri commented Sep 20, 2024

@ACE07-Sev QSD was a recent addition to Cirq and we welcome help with this and other issues. if you (or someone else) would like to help fix this, feel free to assign the issue to yourself and we would be very grateful. otherwise we will discuss this issue in the next cirq-sync.

@ACE07-Sev
Copy link

May I ask how I can get added to the meeting for this? I had a look at the cirq-dev google group, but I can't see any scheduled meetings there.

@NoureldinYosri
Copy link
Collaborator

@ACE07-Sev when you join https://groups.google.com/g/cirq-dev, you get automatic invitation to the meeting

@ACE07-Sev
Copy link

I didn't get any invites hehe, it's ok I'll try to follow from the issue discussion.

@NoureldinYosri
Copy link
Collaborator

@ACE07-Sev I can add you, what is your email?

@ACE07-Sev
Copy link

That would be wonderful. My email is amiralimlk07@gmail.com.

@NoureldinYosri
Copy link
Collaborator

looks like I can't, the event is for members of the group. make sure you followed the steps in https://github.com/quantumlib/Cirq/blob/ce1d9035e878f1f2f73e6ef9575f522bd180665f/docs/dev/rfc_process.md#how-to-submit-an-rfc

if this doesn't work you can join tomorrow at 10am pst through https://meet.google.com/www-wuvf-jjb

@NoureldinYosri NoureldinYosri added triage/discuss Needs decision / discussion, bring these up during Cirq Cynque and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Oct 2, 2024
@NoureldinYosri
Copy link
Collaborator

cirq-sync: @ACE07-Sev do you want to take this task?

@ACE07-Sev
Copy link

Greetings there,

Hope you are well. Sorry for the delay, I am working on it but not sure when I'll reach the fix.

@NoureldinYosri
Copy link
Collaborator

@ACE07-Sev just checking if there are any updates?

@ACE07-Sev
Copy link

@NoureldinYosri Greetings,

Hope you are well. Not yet, I have been working on the QASM conversion issue. I saw your new commit, I'll have a look at that to catch up.

harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
This PR:
- fixes how `_single_qubit_decomposition` handles global phase and reduces the number of operations it returns when possible.
- Makes the eigendecomposition  part resilient to numerical precision issues by:
    1. resorting to the more accurate`eigh` instead of `eig` when the unitary is hermitian.
    2. if the unitary is not hermitian then use Gram–Schmidt to make sure the eigenvectors are orthonormal (mathematically they should but we could get non orthognoal vectors either because of numerical precision or because an eigenvector is repeated and as a result the the eigenvector spanning that subspace are arbitary) 
- Reduce the number of operations by removing operations equaivalent to identity (e.g. rz($\phi$) with $\phi \approx 0$)
- Adds tests for the reported failures making sure they are fixed.
- Adds tests for the corner cases of the decomposition.

---
update: also add `two_qubit_matrix_to_cz_operations` and `three_qubit_matrix_to_operations` as base cases to reduce the depth of the circuit.

---

fixes quantumlib#6666 
fixes quantumlib#6725
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug-report Something doesn't seem to work. triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants