Skip to content

Add 2q fractional gates to the ConsolidateBlocks transpiler pass #13884

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 16 commits into from
Mar 4, 2025

Conversation

ShellyGarion
Copy link
Member

Summary

With the introduction of new 2q-fractional gates, such as RZZGate as part of the QPU, we add them to the ConsolidateBlocks transpiler pass.
https://www.ibm.com/quantum/blog/fractional-gates

This PR should close #13428

Details and comments

Note that this PR may be eventually superseded by #13419

@ShellyGarion ShellyGarion added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Feb 19, 2025
@ShellyGarion ShellyGarion added this to the 2.0.0 milestone Feb 19, 2025
@ShellyGarion ShellyGarion self-assigned this Feb 19, 2025
@ShellyGarion ShellyGarion requested a review from a team as a code owner February 19, 2025 11:56
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@mtreinish mtreinish self-assigned this Feb 24, 2025
@ShellyGarion
Copy link
Member Author

ShellyGarion commented Feb 25, 2025

The only test currently failing is:

def test_no_kak_gates_in_preset_pm(self, opt_level):

This test was added in #13450 to handle issue #13438, and I wonder if we should remove it.
@mtreinish @ElePT - what's your opinion?

the original circuit is:

        ┌────┐   
q_0: ─■─┤ √X ├─■─
      │ ├────┤ │ 
q_1: ─■─┤ √X ├─■─
        └────┘   

the circuit after basis translation is:

global phase: 7π/4
               ┌──────────┐   ┌────┐            ┌──────────┐
q_0: ─■────────┤ Rz(-π/2) ├───┤ √X ├───■────────┤ Rz(-π/2) ├
      │ZZ(π/2) ├──────────┤┌──┴────┴─┐ │ZZ(π/2) ├──────────┤
q_1: ─■────────┤ Rz(-π/2) ├┤ Rx(π/2) ├─■────────┤ Rz(-π/2) ├
               └──────────┘└─────────┘          └──────────┘

The circuit after optimization_level 2 or 3 transpilation is:

global phase: 3π/2
     ┌──────────┐┌───────┐           ┌──────────┐┌────────┐           »
q_0: ┤ Rz(-π/2) ├┤ Rx(π) ├─■─────────┤ Rx(-π/2) ├┤ Rz(-π) ├─■─────────»
     ├─────────┬┘└───────┘ │ZZ(-π/2) ├──────────┤├────────┤ │ZZ(-π/2) »
q_1: ┤ Rz(π/2) ├───────────■─────────┤ Rx(-π/2) ├┤ Rz(-π) ├─■─────────»
     └─────────┘                     └──────────┘└────────┘           »
«     ┌──────────┐
«q_0: ┤ Rz(-π/2) ├
«     ├─────────┬┘
«q_1: ┤ Rz(π/2) ├─
«     └─────────┘ 

The reason that this circuit is being consolidated is that it originally contains CZ gates. If it had contained only RZZ gates, then it would not had been consolidated.

I think that it would be better handled in #13419 (which should take into account the 1-qubit gates).

consolidate_pass = ConsolidateBlocks(basis_gates=["rzz", "rx", "rz"])
res = consolidate_pass(qc)
self.assertEqual({"unitary": 1}, res.count_ops())
self.assertEqual(Operator.from_circuit(qc), Operator(res.data[0].operation.params[0]))
Copy link
Member Author

@ShellyGarion ShellyGarion Feb 25, 2025

Choose a reason for hiding this comment

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

Note that due to the TwoQubitControlledUDecomposer synthesis algorithm, the synthesized circuit is:

q_0: ──────────■──────────────────
     ┌───────┐ │ZZ(-0.3) ┌───────┐
q_1: ┤ Rx(π) ├─■─────────┤ Rx(π) ├
     └───────┘           └───────┘

and not just RZZGate(0.3)

@ShellyGarion ShellyGarion changed the title [WIP] Add 2q fractional gates to the ConsolidateBlocks transpiler pass Add 2q fractional gates to the ConsolidateBlocks transpiler pass Feb 27, 2025
@coveralls
Copy link

coveralls commented Mar 3, 2025

Pull Request Test Coverage Report for Build 13636391692

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 28 of 28 (100.0%) changed or added relevant lines in 4 files are covered.
  • 3509 unchanged lines in 162 files lost coverage.
  • Overall coverage decreased (-1.0%) to 87.105%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/basis/basis_translator/basis_search.rs 1 99.31%
crates/qasm2/src/expr.rs 1 94.23%
qiskit/circuit/classical/types/ordering.py 1 98.25%
qiskit/circuit/controlflow/switch_case.py 1 95.35%
qiskit/circuit/library/standard_gates/r.py 1 97.62%
qiskit/circuit/library/standard_gates/rxx.py 1 97.78%
qiskit/circuit/library/standard_gates/rzx.py 1 97.78%
qiskit/circuit/library/standard_gates/u2.py 1 96.88%
qiskit/circuit/random/utils.py 1 94.87%
qiskit/circuit/store.py 1 96.77%
Totals Coverage Status
Change from base Build 13411473274: -1.0%
Covered Lines: 75855
Relevant Lines: 87085

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks Shelly! The PR looks good, I just left two small comments that should be easy to look into.

use crate::two_qubit_decompose::{TwoQubitBasisDecomposer, TwoQubitControlledUDecomposer};

#[allow(clippy::large_enum_variant)]
#[derive(Clone, Debug, FromPyObject)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any public uses of DecomposerType, so the enum can probably be kept private, right? Similarly, I didn't find where is the FromPyObject trait used, but I might have missed it.

Copy link
Member Author

Choose a reason for hiding this comment

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

FromPyObject is used to call either the TwoQubitBasisDecomposer or the TwoQubitControlledUDecomposer from python, see this comment:
#13884 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

removing pub from the enum produces this warning:
warning: type consolidate_blocks::DecomposerType is more private than the item consolidate_blocks

CXGate(), basis_fidelity=approximation_degree or 1.0
elif kak_param_gates:
self.decomposer = TwoQubitControlledUDecomposer(
KAK_GATE_PARAM_NAMES[kak_param_gates.pop()]
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember a discussion we had in unitary synthesis where we agreed not to use pop because of result reproducibility. I think the same logic would apply in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

done in 9095358

@ShellyGarion ShellyGarion requested a review from ElePT March 3, 2025 17:10
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for answering my questions.

@ElePT ElePT added this pull request to the merge queue Mar 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 4, 2025
@ShellyGarion ShellyGarion added this pull request to the merge queue Mar 4, 2025
github-merge-queue bot pushed a commit that referenced this pull request Mar 4, 2025
…13884)

* add TwoQubitControlledUDecomposer to init file

* add kak parametrized gates

* add a test for parametrized gates

* add num_basis_gates_inner function to TwoQubitControlledUDecomoser class

* add TwoQubitControlledUDecomposer to consolidate_blocks function

* add TwoQubitControlledUDecomposer to ConsolidateBlocks pass

* add FromPyObject to enum

* replace _inner_decomposition by _inner_decomposer

* update rust code

* add self.gate_name to 2-qubit decmposer classes

* extend test_collect_rzz test

* add release notes

* update test_no_kak_gates_in_present_pm

* remove commented line

* add allow clippy

* do not pop
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 4, 2025
@ElePT ElePT added this pull request to the merge queue Mar 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 4, 2025
@ShellyGarion ShellyGarion added this pull request to the merge queue Mar 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 4, 2025
@mtreinish mtreinish added this pull request to the merge queue Mar 4, 2025
@mtreinish
Copy link
Member

For those worried that this bounced off the merge queue twice, looking at the log those failures were caused by issues with the latest aer release and not issues in this pr itself.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 4, 2025
@mtreinish mtreinish added this pull request to the merge queue Mar 4, 2025
Merged via the queue into Qiskit:main with commit ec33b5f Mar 4, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: transpiler Issues and PRs related to Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize consecutive RZZ gates into a single RZZ gate
5 participants