Cirq icon indicating copy to clipboard operation
Cirq copied to clipboard

Fix circuit reversal in stratified_circuit in cirq-core/cirq/transformers/stratify.py

Open wang-anthony03 opened this issue 5 months ago • 10 comments

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.

wang-anthony03 avatar Jul 24 '25 03:07 wang-anthony03

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.

google-cla[bot] avatar Jul 24 '25 03:07 google-cla[bot]

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.

wang-anthony03 avatar Jul 24 '25 03:07 wang-anthony03

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.

daxfohl avatar Jul 25 '25 13:07 daxfohl

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.

wang-anthony03 avatar Jul 25 '25 20:07 wang-anthony03

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.

daxfohl avatar Jul 25 '25 21:07 daxfohl

@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?

mhucka avatar Aug 07 '25 19:08 mhucka

I am removing the before-1.6.1 label as this does not seem critical for the patch release.

pavoljuhas avatar Aug 13 '25 02:08 pavoljuhas

Sorry, was a bit busy the past weeks, going to fix the testing issues and send in an update.

wang-anthony03 avatar Sep 01 '25 23:09 wang-anthony03

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.

codecov[bot] avatar Sep 10 '25 00:09 codecov[bot]

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

github-actions[bot] avatar Dec 10 '25 00:12 github-actions[bot]