-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix nondeterminacy in Circuit.insert (simplified) #7043
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7043 +/- ##
=======================================
Coverage 98.16% 98.16%
=======================================
Files 1093 1093
Lines 95466 95523 +57
=======================================
+ Hits 93711 93767 +56
- Misses 1755 1756 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a small suggestion.
Thank you for taking care of this!
Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Added BREAKING label. It doesn't break any unit test, though it does change circuit construction behavior. Those changes are specified in the 2nd paragraph of the description above. The two changes involving order dependency were previously nondeterministic, and this change makes them deterministic. The other change is the example |
* Fix nondeterminacy and a couple other issues in Circuit.insert * code comments * code comments * code comments * test1 and test7 reverted * remove no longer relevant new tests * Update cirq-core/cirq/circuits/circuit.py Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com> * Update circuit.py * Put back the last line of docstring --------- Co-authored-by: Pavol Juhas <juhas@google.com> Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
#6997 was a PR to fix all edge cases of #6986, but the behavior change was significant enough to be concerning. This PR replaces it with the minimum changes to fix only those edge cases that are outright bugs, without breaking any existing tests or causing possible behavior changes to 'oddities'.
Fixes #6711, and fixes #6986 edge cases that caused the bug due to order dependency:
test_3
will now produce a result equal to that oftest_4
, andtest_5
output will now equal that oftest_6
. Also fixes the bugtest_2
where EARLIEST is not performed. The behaviors intest_1
andtest_7
will remain as-is for now, and we can decide later whether we want to change those behaviors or not. (This PR description will close that issue; if we want to change those behaviors, open a new issue specific to them).Implementation description from #6997 still applies:
The diff from #6997 can be seen in commit 577f9c0