Skip to content

Fix bug in circuit.insert.#7823

Open
codrut3 wants to merge 1 commit intoquantumlib:mainfrom
codrut3:issue-7611-c
Open

Fix bug in circuit.insert.#7823
codrut3 wants to merge 1 commit intoquantumlib:mainfrom
codrut3:issue-7611-c

Conversation

@codrut3
Copy link
Copy Markdown
Contributor

@codrut3 codrut3 commented Jan 3, 2026

Change _group_into_moment_compatible to take into account measurement and control keys. Otherwise two incompatible operations can be put in the same moment.
I also changed _can_add_op_at to look at measurement and control keys.
I added unit tests that show the issue.

Change _group_into_moment_compatible to take into account measurement
and control keys.
@codrut3 codrut3 requested review from a team and vtomole as code owners January 3, 2026 10:28
@codrut3 codrut3 requested a review from mpharrigan January 3, 2026 10:28
@github-actions github-actions Bot added the size: M 50< lines changed <250 label Jan 3, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.57%. Comparing base (ceb9432) to head (b11b2de).
⚠️ Report is 128 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7823      +/-   ##
==========================================
- Coverage   99.57%   99.57%   -0.01%     
==========================================
  Files        1102     1102              
  Lines       98772    98828      +56     
==========================================
+ Hits        98352    98407      +55     
- Misses        420      421       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@daxfohl
Copy link
Copy Markdown
Collaborator

daxfohl commented Jan 4, 2026

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.

@codrut3
Copy link
Copy Markdown
Contributor Author

codrut3 commented Jan 5, 2026

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

not op_measurement_keys.isdisjoint(moment_measurement_keys)

@daxfohl
Copy link
Copy Markdown
Collaborator

daxfohl commented Jan 6, 2026

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).

@pavoljuhas
Copy link
Copy Markdown
Collaborator

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.

@codrut3
Copy link
Copy Markdown
Contributor Author

codrut3 commented Jan 11, 2026

Thank you! I created #7829

@github-actions
Copy link
Copy Markdown

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.

@github-actions github-actions Bot added the status/stale Closed due to inactivity for an extended period of time label Apr 12, 2026
@mhucka mhucka removed the status/stale Closed due to inactivity for an extended period of time label Apr 15, 2026
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.

4 participants