Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.63%. Comparing base (3164943) to head (c4e0106).
⚠️ Report is 5 commits behind head on main.

@@            Coverage Diff             @@
##             main    #7823      +/-   ##
==========================================
- Coverage   99.63%   99.63%   -0.01%     
==========================================
  Files        1110     1110              
  Lines      100072   100136      +64     
==========================================
+ Hits        99710    99773      +63     
- Misses        362      363       +1     

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

Allowing multiple measurements to the same key in the same moment was a requirement from the HW team. Unless that has changed, this would be breaking.

Thank you for pointing this out @daxfohl ! Doesn't this mean this condition here is also wrong:

Maybe. Though it can't be removed completely, because then it could fall through to earlier moments and break commutativity. (There's a lot of https://xkcd.com/1172/ in the circuit construction code).

cirq-cync - @codrut3 - please create an issue for this PR, e.g., to request that measurement and control keys cannot overlap in the same moment. This would be potentially a breaking change so request feedback from Ilya Drozdov and Christopher Wood for the issue to comment if the current behavior is needed in some situations.

Thank you! I created #7829

This pull request has been automatically labeled as stale because 90 days have passed without activity. If no further activity occurs and the status/stale label is not removed by a maintainer within 60 days, this pull request will be closed. If you want to keep the pull request open, leave a comment here and the status will be updated automatically.

If you have questions or feedback about this process, please open a new issue in this repository to let us know. You can also send email to the maintainers at quantum-oss-maintainers@google.com.

@codrut3 - would you be available to update this along the last comments in #7829 ?

@pavoljuhas Thank you for checking! I updated the PR by allowing operations with the same measurement key in the same moment, in the context of the methods changed by this PR (_can_add_op_at, _group_into_moment_compatible).

However, there is older code in circuit.py where one checks that measurement keys are disjoint: for example in _PlacementCache, or here. Should I remove these checks to be consistent everywhere?

Re #7823 (comment)

Allowing multiple measurements to the same key in the same moment was a requirement from the HW team.

There is a distinction in Cirq between MeasurementGate acting on 2 qubits with common key, Moment(cirq.measure(q0, q1, key="k")) and 2 single-qubit measurement gates, i.e., Moment(cirq.measure(q0, key="k"), cirq.measure(q1, key="k")). In the first case cirq simulation allows to recover results for both qubits, in the second case the result for q0 seems overwritten by q1.

Do we need to allow that case or can HW team do with the first scenario?

cynq discussion - let us do a check for overlapping measurement keys in Circuit.insert, but first verify if it breaks anything in the internal code. Circuits should use cirq.measure(q0, q1, key="k") to setup multi-qubit measurement with the same key.

• @pavoljuhas - confirm this does not break tests in the internal code.

I have restored the comparison with measurement keys in 42c13d6 and ran unit tests (those which use cirq) for the internal code at I9765f3a55b8fa0e9e08bf72ce918fb46606af871. There were no new failures or noticeable difference w/r to using main-branch Cirq.

Once this is merged and out in a dev-release, I will run a full internal test suite on the CI to be sure this is not breaking anything.

Confirming the internal CI passed successfully with this change at q-cl/150609 (not submitted, test only).

Adding BREAKING CHANGE label for an unlikely case this changes someone's circuits built with Circuit.insert.