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

Fix commutation of classical controls #4872

Merged
merged 12 commits into from
Jan 28, 2022

Conversation

daxfohl
Copy link
Collaborator

@daxfohl daxfohl commented Jan 22, 2022

Fixes #4870: Adds logic to ensure classically controlled ops do not commute with measurements of the same key (fixes #4880). Also hardens the logic in circuit.append to apply this logic bidirectionally (fixes #4882 too).

@daxfohl daxfohl requested review from cduck, vtomole and a team as code owners January 22, 2022 18:07
@CirqBot CirqBot added the size: S 10< lines changed <50 label Jan 22, 2022
@daxfohl daxfohl requested a review from maffoo January 22, 2022 18:07
@MichaelBroughton
Copy link
Collaborator

Does this also fix issues with optimizers and decompose ?

@daxfohl
Copy link
Collaborator Author

daxfohl commented Jan 22, 2022

I can't make a blanket statement, but most optimizers should either use commutes protocol or circuit.append, and those would be fixed.

I'm not aware of an issue with decompose.

@MichaelBroughton
Copy link
Collaborator

MichaelBroughton commented Jan 22, 2022

Running the test snippet from #4870 it looks like the operations are still hopping over one another and the simulation is breaking. Am I missing something ?

q = cirq.LineQubit.range(2)
c = cirq.Circuit(cirq.H(q[0]), cirq.measure(q[0], key='m'), cirq.X(q[1]).with_classical_controls('m'))
print(c)

cirq.SynchronizeTerminalMeasurements().optimize_circuit(c)
print(c)
cirq.Simulator().simulate(c)

@daxfohl
Copy link
Collaborator Author

daxfohl commented Jan 22, 2022

Ah, I didn't realize it's an inplace transformer, so the test I added doesn't do anything. Changing my test to after=circuit.copy() repros the problem.

Anyway, that whole transformer has major issues. It doesn't handle multi-qubit measurements either. The following breaks:

def test_multi_qubit():
    q0, q1 = cirq.LineQubit.range(2)
    circuit = cirq.Circuit(
        cirq.measure(q0, q1, key='m'), cirq.H(q1)
    )
    assert_optimizes(before=circuit, after=circuit.copy())

That said, I made the same error in the alight_right test, and after fixing the test, it turns red again. This time the problem is just that Moment._control_keys_ is not defined. Pushing a fix for that in a sec.

@daxfohl
Copy link
Collaborator Author

daxfohl commented Jan 22, 2022

I'll add some extra tests to the commutes protocol before merging.

@tanujkhattar tanujkhattar self-assigned this Jan 23, 2022
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.

LGTM % nit.

cirq-core/cirq/ops/raw_types.py Show resolved Hide resolved
@tanujkhattar tanujkhattar merged commit 2b2a6fe into quantumlib:master Jan 28, 2022
@daxfohl daxfohl deleted the fixcommute branch January 29, 2022 22:33
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
* Fix commutation of classical controls

* Add moment.control_keys protocol

* Improve handling of classical controls in commutation

* move align tests
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
* Fix commutation of classical controls

* Add moment.control_keys protocol

* Improve handling of classical controls in commutation

* move align tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
4 participants