Skip to content

Simplify decomposition of controlled eigengates with global phase #7383

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Jun 25, 2025
Merged

Simplify decomposition of controlled eigengates with global phase #7383

merged 15 commits into from
Jun 25, 2025

Conversation

daxfohl
Copy link
Collaborator

@daxfohl daxfohl commented May 29, 2025 •
edited
Loading

Fixes #7238

  1. Create a flag in DecomposeContext to configure extraction of terminal gates' global phases into distinct GlobalPhaseGates.
    a. This flag defaults to False for backward compatibility.
  2. Instrument the terminal decomposition gates (X, Y, Z, CZ) to check this flag and extract global phase if it exists.
    a. Added some convenience methods in GlobalPhaseGate to help.
    b. Updated some unrelated existing code to use these convenience methods for clarity.
  3. Update ControlledGate.decompose to attempt decompose the subgate before attempting the brute-force approach.
    a. This is what ultimately simplifies the decomposition result.
    b. Step 2 also allowed removal of the entire CZPowGate instanceof block there, since it was about extracting global phase.
  4. Add unit tests for each change.

@daxfohl daxfohl requested review from vtomole and a team as code owners May 29, 2025 05:04
@daxfohl daxfohl requested a review from 95-martin-orion May 29, 2025 05:04
@github-actions github-actions bot added the size: L 250< lines changed <1000 label May 29, 2025
@daxfohl daxfohl changed the title Remove controlled cz case Simplify decomposition of controlled eigengates with global phase May 29, 2025
@daxfohl daxfohl mentioned this pull request May 29, 2025
Closed
@codecov Codecov
Copy link

codecov bot commented May 29, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.69%. Comparing base (cac6aad) to head (59fd6d2).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #7383   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files        1112     1112           
  Lines       98076    98152   +76     
=======================================
+ Hits        96798    96875   +77     
+ Misses       1278     1277    -1

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

@codrut3 codrut3 mentioned this pull request Jun 1, 2025
Merged
@NoureldinYosri NoureldinYosri self-assigned this Jun 4, 2025
daxfohl
daxfohl commented Jun 10, 2025
View reviewed changes
daxfohl
daxfohl commented Jun 10, 2025
View reviewed changes
daxfohl
daxfohl commented Jun 10, 2025
View reviewed changes
daxfohl
daxfohl commented Jun 10, 2025
View reviewed changes
daxfohl
daxfohl commented Jun 10, 2025
View reviewed changes
@daxfohl daxfohl marked this pull request as draft June 14, 2025 20:54
@daxfohl daxfohl marked this pull request as ready for review June 16, 2025 18:53
github-merge-queue bot pushed a commit that referenced this pull request Jun 17, 2025
@codrut3
Change merge_single_qubit_moments_to_phxz to support global phase. (#…
a839837 
…7405)

This addresses an issue I found while working on #7291 

Currently `merge_single_qubit_moments_to_phxz` will not merge moments
that have a global phase, meaning that moments that could theoretically
be merged are not. I updated it so that global phase is also supported.
I added two tests to show the issue.

Perhaps this situation is not common right now, but if #7383 is
committed and the flag is used, it may become more common. @daxfohl

Note that I'm not merging the global phase operations in a single one,
because I'm thinking some phases could be parametrized; should I attempt
to do this?
BichengYing pushed a commit to BichengYing/Cirq that referenced this pull request Jun 20, 2025
@codrut3 @BichengYing
Change merge_single_qubit_moments_to_phxz to support global phase. (q…
676157f 
…uantumlib#7405)

This addresses an issue I found while working on quantumlib#7291 

Currently `merge_single_qubit_moments_to_phxz` will not merge moments
that have a global phase, meaning that moments that could theoretically
be merged are not. I updated it so that global phase is also supported.
I added two tests to show the issue.

Perhaps this situation is not common right now, but if quantumlib#7383 is
committed and the flag is used, it may become more common. @daxfohl

Note that I'm not merging the global phase operations in a single one,
because I'm thinking some phases could be parametrized; should I attempt
to do this?
NoureldinYosri
NoureldinYosri reviewed Jun 24, 2025
View reviewed changes
Copy link
Collaborator

@NoureldinYosri NoureldinYosri left a comment

Choose a reason for hiding this comment

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

overall LGTM%nits

daxfohl and others added 4 commits June 24, 2025 11:53
@daxfohl daxfohl requested a review from NoureldinYosri June 25, 2025 03:26
NoureldinYosri
NoureldinYosri approved these changes Jun 25, 2025
View reviewed changes
Copy link
Collaborator

@NoureldinYosri NoureldinYosri left a comment

Choose a reason for hiding this comment

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

thanks @daxfohl

@NoureldinYosri NoureldinYosri added this pull request to the merge queue Jun 25, 2025
Merged via the queue into quantumlib:main with commit cedf227 Jun 25, 2025
35 checks passed
BichengYing pushed a commit to BichengYing/Cirq that referenced this pull request Jun 25, 2025
@daxfohl @NoureldinYosri @BichengYing
Simplify decomposition of controlled eigengates with global phase (qu…
b90cea8 
…antumlib#7383)

Fixes quantumlib#7238

1. Create a flag in `DecomposeContext` to configure extraction of
terminal gates' global phases into distinct GlobalPhaseGates.
    a. This flag defaults to False for backward compatibility.
2. Instrument the terminal decomposition gates (X, Y, Z, CZ) to check
this flag and extract global phase if it exists.
    a. Added some convenience methods in GlobalPhaseGate to help.
b. Updated some unrelated existing code to use these convenience methods
for clarity.
3. Update `ControlledGate.decompose` to attempt decompose the subgate
before attempting the brute-force approach.
    a. This is what ultimately simplifies the decomposition result.
b. Step 2 also allowed removal of the entire CZPowGate instanceof block
there, since it was about extracting global phase.
4. Add unit tests for each change.

---------

Co-authored-by: Noureldin <noureldinyosri@gmail.com>
@daxfohl daxfohl deleted the remove-controlled-cz-case branch June 26, 2025 02:38
@YaegakiY YaegakiY mentioned this pull request Jul 7, 2025
Closed
ddddddanni pushed a commit to ddddddanni/Cirq that referenced this pull request Jul 15, 2025
@codrut3 @ddddddanni
Change merge_single_qubit_moments_to_phxz to support global phase. (q…
4ad289c 
…uantumlib#7405)

This addresses an issue I found while working on quantumlib#7291 

Currently `merge_single_qubit_moments_to_phxz` will not merge moments
that have a global phase, meaning that moments that could theoretically
be merged are not. I updated it so that global phase is also supported.
I added two tests to show the issue.

Perhaps this situation is not common right now, but if quantumlib#7383 is
committed and the flag is used, it may become more common. @daxfohl

Note that I'm not merging the global phase operations in a single one,
because I'm thinking some phases could be parametrized; should I attempt
to do this?
ddddddanni pushed a commit to ddddddanni/Cirq that referenced this pull request Jul 15, 2025
@daxfohl @NoureldinYosri @ddddddanni
Simplify decomposition of controlled eigengates with global phase (qu…
0b04110 
…antumlib#7383)

Fixes quantumlib#7238

1. Create a flag in `DecomposeContext` to configure extraction of
terminal gates' global phases into distinct GlobalPhaseGates.
    a. This flag defaults to False for backward compatibility.
2. Instrument the terminal decomposition gates (X, Y, Z, CZ) to check
this flag and extract global phase if it exists.
    a. Added some convenience methods in GlobalPhaseGate to help.
b. Updated some unrelated existing code to use these convenience methods
for clarity.
3. Update `ControlledGate.decompose` to attempt decompose the subgate
before attempting the brute-force approach.
    a. This is what ultimately simplifies the decomposition result.
b. Step 2 also allowed removal of the entire CZPowGate instanceof block
there, since it was about extracting global phase.
4. Add unit tests for each change.

---------

Co-authored-by: Noureldin <noureldinyosri@gmail.com>
This was referenced Aug 5, 2025
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NoureldinYosri NoureldinYosri NoureldinYosri approved these changes

@vtomole vtomole Awaiting requested review from vtomole vtomole is a code owner

@95-martin-orion 95-martin-orion Awaiting requested review from 95-martin-orion 95-martin-orion is a code owner automatically assigned from quantumlib/cirq-maintainers

Assignees

@NoureldinYosri NoureldinYosri

Labels
size: L 250< lines changed <1000
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Simplify decomposition of controlled eigengates with global phase
2 participants
@daxfohl @NoureldinYosri