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 in Cirq Interop: Attempt 2 #1100

Merged
merged 8 commits into from
Jul 8, 2024
Merged

Conversation

tanujkhattar
Copy link
Collaborator

@tanujkhattar tanujkhattar commented Jul 1, 2024

Another attempt at #963. Opened a new PR instead of rebasing that one since a major reorganization has occurred since that attempt.

High level updates are:

  • cirq.inverse doesn't play well with qubit allocation / deallocation. Therefore, removed decompose_from_registers from the Adjoint bloq and default to using the bloq-style decompose_bloq API
  • InteropQubitManager had a bug where the managed qubits could be allocated twice (eg: by the underlying greedy qubit manager)

The above two were the primary reason for #818, which is now fixed. As part of these upgrades, a few other minor improvements were done. For example:

  • SGate(is_adjoint=True).as_cirq_op() was returning cirq.S instead of cirq.S**-1. This is now fixed.
  • Allocate and Free now have a as_cirq_op defined and won't show up as explicit operations in the cirq circuit.
  • Comparison bloqs are made self adjoint because tests of ReflectionUsingPrepare rely on not decomposing LessThanEqual gate so the cirq simulator can make sure of the optimized implementation of apply_unitary for cirq.ArithmeticGate's. This gets lost if we have Adjoint(LessThanEqual) instead of LessThanEqual in the circuit. Flattening out the entire cirq circuit for ReflectionUsingPrepare -> StatePreparationViaAliasSampling -> LessThanEqual is catastrophic for performance. cc @mpharrigan It'll be nice to see how the new tensor network simulation perform for this specific test. It'll be nice to get rid of construct_gate_helper_and_qubit_order and rewrite get_3q_uniform_dirac_notation test with the same parameters but using tensor network simulations.

Currently based on top of #1099

The only pending thing in this PR is that tests for SelectSwapQROM are still failing; and the failure is related to the one seen in #961. More investigations are needed here to figure out whats going wrong.

@mpharrigan
Copy link
Collaborator

I think some of the selectswapqrom tests have zero-bitsize registers that have snuck in. Maybe that could be causing a problem?

@tanujkhattar
Copy link
Collaborator Author

@mpharrigan The SelectSwapQROM failure was because SGate(is_adjoint=True).as_cirq_op() was returning cirq.S instead of cirq.S**-1 :(

All tests are now fixed and this is ready for a review. I've also added more simulation tests and I request everyone to add full state vector simulation tests as part of the test suite so we can catch such bugs earlier. Classical simulation was giving correct answer despite full state vector simulation being wrong; so one cannot replace the other from our perspective.

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

thank you!

@mpharrigan mpharrigan merged commit 3315602 into quantumlib:main Jul 8, 2024
7 checks passed
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.

2 participants