[WIP] Add 2q fractional gates to the `UnitarySynthesis` tranpiler pass #13568

ShellyGarion · 2024-12-16T11:06:40Z

Summary

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

Details and comments

qiskit-bot · 2024-12-16T11:06:45Z

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

@Qiskit/terra-core
@levbishop

coveralls · 2024-12-16T13:24:13Z

Pull Request Test Coverage Report for Build 12455380316

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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

24 of 24 (100.0%) changed or added relevant lines in 3 files are covered.
24 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.007%) to 88.968%

Files with Coverage Reduction	New Missed Lines	%
qiskit/circuit/library/quantum_volume.py	1	93.18%
crates/accelerate/src/unitary_synthesis.rs	1	92.2%
crates/qasm2/src/expr.rs	1	94.02%
crates/qasm2/src/lex.rs	4	91.98%
crates/qasm2/src/parse.rs	6	97.62%
qiskit/quantum_info/states/stabilizerstate.py	11	93.89%

Totals
Change from base Build 12296775949:	0.007%
Covered Lines:	79447
Relevant Lines:	89298

💛 - Coveralls

gadial · 2024-12-17T07:44:15Z

qiskit/transpiler/passes/synthesis/unitary_synthesis.py

@@ -107,7 +129,12 @@ def _choose_kak_gate(basis_gates):
    """Choose the first available 2q gate to use in the KAK decomposition."""


Question: It seems we are not choosing the element according to some order, but using set pop, which is random, leading to nondeterministic behavior. Can this be avoided (or already avoided and I'm missing something)? e.g.

kak_gate = sorted([KAK_GATE_PARAM_NAMES[kak_gate_param] for kak_gate_params in kak_gates_params])[0]

This is a good point @gadial, I had always assumed that this randomness was a reflection of the fact that there can be several equally valid gates. And if that's case, I wonder if we should favor the one that comes first in alphabetical order? At the same time, I am not sure how conscious some of these decisions were. There are other parts of the code (the parts that were migrated to Rust) where we actually rely on insertion order when there are several valid decompositions, so at that point I guess it's as arbitrary as sorting alphabetically. It might be also worth noting that these cases don't come up often in real scenarios because we rarely see a basis set with more than one 2q gate (although it can definitely happen).

Thanks for the comment. I'll look into it.

Thank you @ElePT. My approach is usually to prefer the arbitrary to the nondeterminisitic, since nondeterministic behavior makes testing more difficult (since it's harder to reproduce problems) but also less resilient (since we are only testing the conditions generated nondeterminisically on the current testing machines, whereas a future version of Python might behave differently).

I agree with @gadial's comment. If the user provides several 2q KAK gates, this "pop" just randomly chooses one of them. We would probably want a more deterministic behavior (perhaps favoring more "commonly" used 2q gates).

I think that sounds reasonable, it helps with benchmarking and bug-catching.

done in eeff4cd

ElePT

Hi @ShellyGarion! thanks for opening the PR, I left a couple of preliminary comments. I hope they make sense :)

ElePT · 2024-12-17T13:31:50Z

qiskit/transpiler/passes/synthesis/unitary_synthesis.py

@@ -107,7 +129,12 @@ def _choose_kak_gate(basis_gates):
    """Choose the first available 2q gate to use in the KAK decomposition."""


This is a good point @gadial, I had always assumed that this randomness was a reflection of the fact that there can be several equally valid gates. And if that's case, I wonder if we should favor the one that comes first in alphabetical order? At the same time, I am not sure how conscious some of these decisions were. There are other parts of the code (the parts that were migrated to Rust) where we actually rely on insertion order when there are several valid decompositions, so at that point I guess it's as arbitrary as sorting alphabetically. It might be also worth noting that these cases don't come up often in real scenarios because we rarely see a basis set with more than one 2q gate (although it can definitely happen).

ElePT · 2024-12-17T13:33:36Z

qiskit/synthesis/two_qubit/two_qubit_decompose.py

+    def __call__(
+        self, unitary: Operator | np.ndarray, approximate=False, use_dag=False, atol=DEFAULT_ATOL
+    ) -> QuantumCircuit:


I wonder what's the motivation for this change. Are approximate or use_dag used in the code? Could it be an oversight?

when calling the decomposer2q later in the code it assumes to have both parameters, see e.g

qiskit/qiskit/transpiler/passes/synthesis/unitary_synthesis.py

Line 1046 in 17648eb

synth_circ = decomposer2q(su4_mat, approximate=approximate, use_dag=True)

I see. I keep forgetting that we still have python code for the target aware case (this is kept in case someone uses a non-default plugin). In that case I think we should respect the keyword-only argument:

Suggested change

def __call__(

self, unitary: Operator | np.ndarray, approximate=False, use_dag=False, atol=DEFAULT_ATOL

) -> QuantumCircuit:

def __call__(

self, unitary: Operator | np.ndarray, approximate=False, use_dag=False, *, atol=DEFAULT_ATOL

) -> QuantumCircuit:

I also have realized that the TwoQubitControlledUDecomposer is not documented. If we are going to add it to the default pipeline, I believe we should, as right now it's not part of the public interface. So we should probably make sure to add it to the API docs with the other decomposers and add a release note listing the class as a new feature.

I plan to add TwoQubitControlledUDecomposer to the API docs following this PR (as I may need to update the API as part of adding it to the transpiler pass)

ElePT · 2024-12-17T13:44:10Z

qiskit/transpiler/passes/synthesis/unitary_synthesis.py

+    # if isinstance(kak_gate, RZXGate):
+    #    backup_optimizer = TwoQubitBasisDecomposer(
+    #        CXGate(),
+    #        basis_fidelity=basis_fidelity,
+    #        euler_basis=euler_basis,
+    #        pulse_optimize=pulse_optimize,
+    #    )
+    #    decomposer2q = XXDecomposer(euler_basis=euler_basis, backup_optimizer=backup_optimizer)
+
+    if kak_gate in KAK_GATE_PARAM_NAMES.values():
+        decomposer2q = TwoQubitControlledUDecomposer(kak_gate)


Double checking our discussion from earlier in the week: for the target-aware code we discussed adding a third check (apart from is_controlled or is_supercontrolled) to differentiate cases for the 3 available decomposers: TwoQubitDecomposer, XXDecomposer, and TwoQubitControlledUDecomposer. However, for the Python code (just basis gates), it looks like we are simply substituting the XXDecomposer for the TwoQubitControlledUDecomposer. Is this the plan?

In the case of basis gates, I'm not sure if we can have 3 options or only 2 (this is why this code is only commented out and not removed yet).
If a user provides the basis gates [rzz, rz, ry] how can we tell if they want an RZZGate with an arbitrary angle or an RZZGate with a fixed angle (and if so - which fixed value)?

Another comment: I ran the tests in qiskit/test/python/transpiler/test_unitary_synthesis.py (main branch), and couldn't find any test that goes into the following "if" statement (lines 181-188 above), so I wonder if the XXDecomposer here has ever been used.

I am not surprised, the coverage of these tests is not good because a lot of the tests are in the synthesis folder and not testing the transpilation pipeline, but calling the synthesis methods directly. Maybe this is a good opportunity to do some test refactoring before merging this PR. During the Rust migration I also noticed there weren't any tests for the UnitarySynthesis class that used the QSD method with a target and had to add a couple of them. I think it would make sense to ensure that we have a couple of tests per decomposer, and that there are sufficient tests for the target (HW-aware) path (this I could do in a separate PR because I didn't have time to look more into it during the Rust migration).

If a user provides the basis gates [rzz, rz, ry] how can we tell if they want an RZZGate with an arbitrary angle or an RZZGate with a fixed angle?

I think this is up to us to decide and document the behavior change. I am not sure how many users actually rely or expect the fixed angle?

FYI, I started the test cleanup with this PR: #13582, that is not much but I think can be helpful for future development. Let me know if you have any thoughts on it!

Another question is what to expect when given a parametrized gate in the target? see e.g this test.

Currently, we replace the parametrized gates by gates with a constant angle, without even checking if this angle is actually calibrated in the target:

qiskit/qiskit/transpiler/passes/synthesis/unitary_synthesis.py

Line 801 in 079f69f

def _replace_parameterized_gate(op):

Again, I could not find any test that checks the XXDecomposer with a given target, namely the tests don't go into here.

TwoQubitControlledUDecomposer to _decomposer_2q_from_basis_gates

89ca81e

ShellyGarion added the mod: transpiler Issues and PRs related to Transpiler label Dec 16, 2024

ShellyGarion added this to the 2.0.0 milestone Dec 16, 2024

ShellyGarion requested a review from ElePT December 16, 2024 11:06

ShellyGarion requested review from alexanderivrii and a team as code owners December 16, 2024 11:06

ElePT self-assigned this Dec 16, 2024

update (temporarily) basis gates in test

077ecb9

gadial reviewed Dec 17, 2024

View reviewed changes

ElePT reviewed Dec 17, 2024

View reviewed changes

minor fix

18b20f6

ElePT mentioned this pull request Dec 19, 2024

Small refactoring of unitary synthesis tests #13582

Open

ShellyGarion added 4 commits December 19, 2024 06:49

add EulerBasis as a parameter to TwoQubitControlledUDecomposer

967da13

fix global_phase calculation in TwoQubitContolledUDecomposer

3bb10fd

add TwoQubitControlledUDecomposer to the docs

14c7aac

make the choice of kak_gate deterministic

eeff4cd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add 2q fractional gates to the `UnitarySynthesis` tranpiler pass #13568

[WIP] Add 2q fractional gates to the `UnitarySynthesis` tranpiler pass #13568

ShellyGarion commented Dec 16, 2024

qiskit-bot commented Dec 16, 2024

coveralls commented Dec 16, 2024 •

edited

Loading

gadial Dec 17, 2024

ElePT Dec 17, 2024

ShellyGarion Dec 17, 2024

gadial Dec 17, 2024

ShellyGarion Dec 18, 2024

ElePT Dec 18, 2024

ShellyGarion Dec 22, 2024

ElePT left a comment

ElePT Dec 17, 2024

ElePT Dec 17, 2024

ShellyGarion Dec 17, 2024

ElePT Dec 18, 2024

ShellyGarion Dec 18, 2024

ElePT Dec 17, 2024

ShellyGarion Dec 17, 2024 •

edited

Loading

ShellyGarion Dec 18, 2024

ElePT Dec 18, 2024 •

edited

Loading

ElePT Dec 18, 2024

ElePT Dec 19, 2024

ShellyGarion Dec 23, 2024

		@@ -107,7 +129,12 @@ def _choose_kak_gate(basis_gates):
		"""Choose the first available 2q gate to use in the KAK decomposition."""

[WIP] Add 2q fractional gates to the UnitarySynthesis tranpiler pass #13568

Are you sure you want to change the base?

[WIP] Add 2q fractional gates to the UnitarySynthesis tranpiler pass #13568

Conversation

ShellyGarion commented Dec 16, 2024

Summary

Details and comments

qiskit-bot commented Dec 16, 2024

coveralls commented Dec 16, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12455380316

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElePT Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[WIP] Add 2q fractional gates to the `UnitarySynthesis` tranpiler pass #13568

[WIP] Add 2q fractional gates to the `UnitarySynthesis` tranpiler pass #13568

coveralls commented Dec 16, 2024 •

edited

Loading

ShellyGarion Dec 17, 2024 •

edited

Loading

ElePT Dec 18, 2024 •

edited

Loading