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

Update Density Matrix and State Vector Simulators to work when an operation allocates new qubits as part of its decomposition #6108

Merged
merged 25 commits into from Jun 7, 2023

Conversation

senecameeks
Copy link
Collaborator

This PR addresses #6081 for Density Matrix and State Vector simulators and is related to #6040

It productionizes #6040 (comment) and adds more test cases.

@senecameeks senecameeks requested review from a team, vtomole and cduck as code owners May 26, 2023 19:23
@CirqBot CirqBot added the size: M 50< lines changed <250 label May 26, 2023
Copy link
Contributor

@daxfohl daxfohl left a comment

Choose a reason for hiding this comment

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

Just need to add some empty inputs checks and it looks good.

cirq-core/cirq/sim/simulation_state.py Outdated Show resolved Hide resolved
cirq-core/cirq/sim/simulation_state.py Outdated Show resolved Hide resolved
cirq-core/cirq/sim/simulation_state.py Outdated Show resolved Hide resolved
@tanujkhattar tanujkhattar self-assigned this May 30, 2023
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

I'll wait for you to resolve Dax's comments and make the tests pass and then I'll take a look. In the meantime, here is a relevant discussion where I list out a bunch of different cases for "Gates which can allocate new qubits" that should be added as part of tests. #6112 (review)

If that PR is merged before this one, you can reuse the gates for testing the changes that you make to simulators by importing them from unitary_protocol_test.py.

In the meantime, please let me know if you are stuck or have any questions.

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

I'll wait for you to resolve Dax's comments and make the tests pass and then I'll take a look. In the meantime, here is a relevant discussion where I list out a bunch of different cases for "Gates which can allocate new qubits" that should be added as part of tests. #6112 (review)

If that PR is merged before this one, you can reuse the gates for testing the changes that you make to simulators by importing them from unitary_protocol_test.py.

In the meantime, please let me know if you are stuck or have any questions.

cirq-core/cirq/sim/simulation_state.py Outdated Show resolved Hide resolved
cirq-core/cirq/sim/simulation_state.py Outdated Show resolved Hide resolved
for test_simulator in ['cirq.final_state_vector', 'cirq.final_density_matrix']:
test_sim = eval(test_simulator)(test_circuit)
control_sim = eval(test_simulator)(control_circuit)
cirq.testing.assert_allclose_up_to_global_phase(test_sim, control_sim, atol=1e-6)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@daxfohl Do you know if we mess up the global phase as part of simulations ? The factor / kron methods are messing up global phase since if I replace PhaseUsingCleanAncilla(theta=theta, ancilla_bitsize=num_ancilla).on(q) with cirq.MatrixGate(cirq.unitary(PhaseUsingCleanAncilla(theta=theta, ancilla_bitsize=num_ancilla))).on(q); the tests pass using assert np.allclose(test_sim, control_sim) instead of
cirq.testing.assert_allclose_up_to_global_phase(test_sim, control_sim, atol=1e-6).

Ideally, the resulting state vectors and density matrices should be identical and not equal upto global phase, but the factor / kron (or some other interaction of these methods in this PR) seems to mess up the global phase here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, so what is happening is as follows:

state_to_factor = (0.63+0.33j)|00⟩ + (-0.2+0.67j)|10⟩
# StateVectorSimulationState factors the above state into e, r s.t. 
e= (0.88+0.47j)|0⟩
r= 0.71|0⟩ + (0.14+0.69j)|1⟩
# Now, we just ignore `e` and return `r`; which has the wrong global phase. 

@daxfohl Do we not encounter this problem when factoring out measured / reset qubits when we construct SimulationProductState ? I guess we can just multiply the extra phase to correct it, but curious if this is known pattern elsewhere as well? Seems very relevant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I guess we never really "discard" qubits when constructing a SimulationProductState from unentangled states, and therefore this issue doesn't arise?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added a fix in tanujkhattar@1db8ac5

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought I fixed this in #5847. What is the difference in this scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Probably the linalg functions should take an extra bool parameter to force all phase into the reminder, and verify that the extracted axes are left in a classical basis state. All use cases of those functions, that's what they're ultimately trying to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants