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 nondeterminacy and a couple other issues in Circuit.insert #6997

Closed
wants to merge 9 commits into from

Conversation

daxfohl
Copy link
Collaborator

@daxfohl daxfohl commented Jan 29, 2025

Fixes #6711, and fixes #6986 edge cases.

Instead of inserting one operation at a time, it first batches up ops by moment compatibility (see docstring for _group_into_moment_compatible). This way, the insert algorithm can first check whether any op in the batch requires a new moment to be created, and if so, preemptively create that moment before placing any of the operations from that batch. This allows it to avoid the nondeterminacy caused by the one-by-one algorithm, where one op inserted at moment[k] can inadvertently get pushed forward when a later op inserts a new moment at k.

Tests added for all documented edge cases.

Copying the comment from #6986:

I'd say test_7 is the only one we might want to preserve, as it is the documented behavior, though unexpected. test_1 might also fall into the "let's preserve" category, because it doesn't necessarily do the wrong thing, and because Hyrum's law. Though I think if a user reported it as a bug, we'd agree and fix it. The remainder seem like bugs that should be fixed (with the outputs of test_4 and test_6 being the corrected behavior of test_3 and test_5 respectively).

If we do want to change test_7, the algorithm would be "Insert at [i] if available. If not, try [i-1]. Last resort, create new moment at [i] and put it there". All equivalent to "put it at [i], or just before [i] if something is already there."

If we want to remove the behavior change to test_7 and/or test_1 due to backward compatibility, just mention it either here or in the issue. The PR can be adapted to that in just a couple lines.

@daxfohl daxfohl requested review from vtomole and a team as code owners January 29, 2025 06:11
@daxfohl daxfohl requested a review from fdmalone January 29, 2025 06:11
@CirqBot CirqBot added the size: M 50< lines changed <250 label Jan 29, 2025
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.16%. Comparing base (e2de439) to head (a173a48).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6997      +/-   ##
==========================================
- Coverage   98.17%   98.16%   -0.01%     
==========================================
  Files        1087     1087              
  Lines       94722    94795      +73     
==========================================
+ Hits        92992    93060      +68     
- Misses       1730     1735       +5     

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

@daxfohl
Copy link
Collaborator Author

daxfohl commented Feb 7, 2025

Closing for now due to breaking changes. Replacing with #7043. Can revisit later.

@daxfohl daxfohl closed this Feb 7, 2025
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.

Circuit.insert has odd edge cases Inconsistent behavior of Default Insert Strategy
2 participants