Fix circuit reversal in stratified_circuit in cirq-core/cirq/transformers/stratify.py
Fixes #7474.
stratified_circuit reversed circuits by reversing the moments in the circuit, without reversing the operations in the moment, causing measurement errors.
In order to fix this, a circuit.reverse() was created, following #6899, and used in stratified_circuit to ensure no measurement errors occurred because of the reversed circuit.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
View this failed invocation of the CLA check for more information.
For the most up to date status, view the checks section at the bottom of the pull request.
Wasn't sure about modifying in place vs returning a modified circuit, I can switch the approach if returning a modified circuit would be better. Also, I am new to open source contributions, so any input or nitpicks are appreciated.
I'd say call the function "reversed" and make it return a copy, and put it on AbstractCircuit instead of just Circuit. . Also probably do the same thing on Moment, and try to have it maintain the cached member fields instead of regenerating them, to avoid the perf hit. In AbstractCircuit's reversed method, an optional bool "preserve_operation_order_in_moments=False" parameter to make it explicit that the moment internal structure also gets reversed by default.
You can go ahead and update align_right to use this too, but I'd probably not go further than that as far as backporting in a single PR.
Is there a use case for preserving the order of operations in the moments? If not, would a more verbose function name serve this purpose better? I'm planning on opening a new issue to look for other instances where this new reversal can be used, I'll put the align_right change into that pr to maintain granularity.
Yeah, on second thought, don't bother with the extra parameter. If someone wants to just reverse moments then they can use the slice notation, so no need to add explicit support for that here too.
@wang-anthony03 Thank you for your work on this. Would you be able to take a look at the test failures and address the problems?
I am removing the before-1.6.1 label as this does not seem critical for the patch release.
Sorry, was a bit busy the past weeks, going to fix the testing issues and send in an update.
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 99.37%. Comparing base (9f3b2c5) to head (d2c0bb0).
:warning: Report is 96 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #7531 +/- ##
==========================================
- Coverage 99.56% 99.37% -0.20%
==========================================
Files 1088 1078 -10
Lines 96083 96150 +67
==========================================
- Hits 95667 95546 -121
- Misses 416 604 +188
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
This pull request has been automatically labeled as stale because 90 days have passed without comments or other 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 would like to restore its active status, please leave a comment here; doing so will cause the staleness handler to remove the label.
If you have questions or feedback about this process, we welcome your input. You can open a new issue to let us know (please also reference this issue there, for continuity), or reach out to the project maintainers at [email protected].