Skip to content

Compensate for global phase in MatrixGate decomposition #7118

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

Merged
merged 10 commits into from
Apr 4, 2025

Conversation

daxfohl
Copy link
Collaborator

@daxfohl daxfohl commented Mar 4, 2025

Following up on #7112 (which is the first commit in this PR), this PR fixes the issue at its root: it adds a compensating GlobalPhaseOperation when decomposing a MatrixGate. This allows the controlled decomposition tests to use np.testing.assert_allclose as desired, and also allows controlled MatrixGates to be used in decompositions, so that restriction has been removed.

I think this closes #6523, as the core of that issue is being able to decompose controlled MatrixGates, and the specific analytical decomposition methods do not state that they might return any global phase ops, and doing so could be a breaking change. So, this PR fixes the core issue, but with lower likelihood of introducing a breaking change.

cc @pavoljuhas

@CirqBot CirqBot added the size: M 50< lines changed <250 label Mar 4, 2025
@daxfohl
Copy link
Collaborator Author

daxfohl commented Mar 4, 2025

Looks like this creates a global phase in one of the ionq tests, which is not a supported gate in their gateset. I added it to their gateset just to see if it would allow the test to pass (pending). @tanujkhattar what's the best thing to do here? Add it to their gateset? Create a drop_global_phase_ops transformer and add it to their postprocessor? Add a DecomposeArgs field to ignore global phase? Skip this change entirely?

Copy link

codecov bot commented Mar 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.65%. Comparing base (5a81b3d) to head (056beff).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7118      +/-   ##
==========================================
- Coverage   98.65%   98.65%   -0.01%     
==========================================
  Files        1106     1106              
  Lines       95870    95885      +15     
==========================================
+ Hits        94583    94592       +9     
- Misses       1287     1293       +6     

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

np.testing.assert_allclose(cirq.unitary(cgate), cirq.unitary(circuit), atol=1e-1)
first_op = cirq.IdentityGate(qid_shape=shape).on(*qids) # To ensure same qid order
circuit = cirq.Circuit(first_op, *decomposed)
np.testing.assert_allclose(cirq.unitary(cgate), cirq.unitary(circuit), atol=1e-13)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm the test failure in #7071 (comment) does not happen after this change.

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the test failure on my desktop that happened due to phase difference.
I'd defer to @tanujkhattar if this is an OK change for ionq.

@daxfohl daxfohl marked this pull request as ready for review March 13, 2025 19:05
@daxfohl daxfohl requested review from dabacon, vtomole and a team as code owners March 13, 2025 19:05
@daxfohl daxfohl requested a review from tanujkhattar March 13, 2025 23:41
@daxfohl
Copy link
Collaborator Author

daxfohl commented Apr 4, 2025

@tanujkhattar do you have a recommendation here? This PR compensates for global phase in MatrixGate decomposition, fixing the associated issues regarding the broken decomposition of certain controlled gates that neglect the controlled phase. The change adds global phase ops to compensate, which is straightforward enough, but the remaining concern is that global phase op is not part of the ionq gateset, so the addition of this op during decomposition causes some of their unit tests to break.

For now, to allow their tests to pass, I added global phase ops to their gateset. Is this okay or is there something else we should do? One alternative is we could add a postprocessor to their gateset rules that drops global phase ops from the final circuit (seems straightforward). Alternately we could add a flag to decompose that ignores global phase (not ideal: likely very invasive / backwards-incompatible). Or we could leave things like this with global phase as part of their gateset (preferred). cc @dabacon if there's a direct ionq contact I should be pinging.

@pavoljuhas pavoljuhas added the priority/before-1.5 Finish before the Cirq 1.5 release label Apr 4, 2025
@tanujkhattar
Copy link
Collaborator

This PR compensates for global phase in MatrixGate decomposition, fixing the associated issues regarding the broken decomposition of certain controlled gates that neglect the controlled phase.

I think this is a much needed fix and I'm very happy to see this finally happening. Thanks for doing this!

For now, to allow their tests to pass, I added global phase ops to their gateset. Is this okay or is there something else we should do?

I think this is fine and we can merge the PR as is. Thanks!

@daxfohl daxfohl enabled auto-merge April 4, 2025 23:23
@daxfohl daxfohl added this pull request to the merge queue Apr 4, 2025
Merged via the queue into quantumlib:main with commit a68adcd Apr 4, 2025
38 checks passed
@daxfohl daxfohl deleted the fix-controlled-decompose-test2 branch April 4, 2025 23:56
BichengYing pushed a commit to BichengYing/Cirq that referenced this pull request Jun 20, 2025
)

* Change `assert_allclose` to `equal_upto_global_phase` in test, due to MatrixGate decomposition.

* Add compensating global phase op if needed to MatrixGate decomposition.

* Fix ionq test

* address PR comments

* Extract phase_delta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/before-1.5 Finish before the Cirq 1.5 release size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preserve global phase in analytical decompositions
4 participants