Oxidize `UnitarySynthesis` path using `basis_gates` #13704

ElePT · 2025-01-21T13:37:20Z

Summary

This PR is a compilation of small improvements to the UnitarySynthesis pass, including:

Performance -> Addition of fast path (Rust) for the use case where target is None, all input combinations can now be run using the fast path.
Fixes -> Fixed an oversight where the pulse_optimize argument wasn't added to the fast path and therefore ignored (discovered after expanding the unit tests).

These set the stage for future refactorings of the UnitarySynthesis class to take better advantage of the plugin model (the default plugin is currently ignored in the fast path).

Note that this PR touches on code that is currently actively worked on in:

And builds on top of #13706 (will need to be rebased after it's merged).

Details and comments

coveralls · 2025-01-22T10:29:04Z

Pull Request Test Coverage Report for Build 13369594575

Details

553 of 800 (69.13%) changed or added relevant lines in 3 files are covered.
31 unchanged lines in 6 files lost coverage.
Overall coverage decreased (-0.07%) to 88.187%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/unitary_synthesis.rs	430	444	96.85%
qiskit/transpiler/passes/synthesis/default_unitary_synth_plugin.py	118	351	33.62%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/two_qubit_decompose.rs	1	91.82%
crates/circuit/src/dag_node.rs	3	76.59%
crates/accelerate/src/unitary_synthesis.rs	4	94.39%
crates/qasm2/src/lex.rs	5	92.48%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	6	74.05%
crates/qasm2/src/parse.rs	12	97.15%

Totals
Change from base Build 13336190323:	-0.07%
Covered Lines:	78742
Relevant Lines:	89290

💛 - Coveralls

alexanderivrii

I have taken a quick look at the PR, and it seems to be in a very good shape. I have left a few minor questions, and will take a deeper look once the PRs it's based on are merged. And of course you are missing release notes.

crates/accelerate/src/unitary_synthesis.rs

alexanderivrii · 2025-02-04T12:28:15Z

crates/accelerate/src/unitary_synthesis.rs

-                )?;
-                let out_qargs = dag.get_qargs(packed_instr.qubits);
-                apply_synth_dag(py, &mut out_dag, out_qargs, &synth_dag)?;
+                if basis_gates.is_empty() && matches!(target, None) {


Minor question: is there a benefit of writing matched!(target, None) over target.is_none()?

I don't think so, this was me just being rusty in Rust when I wrote it :) I actually prefer target.is_none()

crates/accelerate/src/unitary_synthesis.rs

qiskit/transpiler/passes/synthesis/unitary_synthesis.py

1ucian0 · 2025-02-10T14:39:35Z

Removing on hold as #13706 merged.

qiskit-bot · 2025-02-10T22:26:10Z

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

@Qiskit/terra-core

… in logic, add comments and docstrings

…te logic from UnitarySynthesis pass. This plugin is currently ONLY USED as a fallback method if a custom unitary synthesis plugin is provided and fails. We don't have any unit tests currently covering this scenario. For more context, the _run_main_loop method in UnitarySynthesis has been replaced by a Rust function when the plugin is set to default.

ElePT · 2025-02-11T12:43:06Z

Sorry @alexanderivrii if I have complicated the review with the last couple of commits. It might look like a lot of changes but I mostly moved around existing code (the one you already reviewed). As a summary:

I restructured the Rust file to try to make it more understandable and reduce code repetition, I also added comments.
On the Python side, I moved the DefaultUnitarySynthesis plugin code to an independent file. This plugin duplicates the Rust logic, which unfortunately was developed in parallel to the plugin infrastructure. The Rust code combines the pass run_main_loop logic with the plugin run and currently cannot simply replace the plugin run. Ideally, we could yet again refactor the code to match the plugin infrastructure, but I don't think this will be possible before 2.0 (in terms of time).

For reference, the Python DefaultUnitarySynthesis class is still set as a fallback for the case where a custom plugin fails, which is the reason why I haven't removed it either. At least separating it from UnitarySynthesis might explain a bit better the situation.

ShellyGarion

@ElePT - thanks for refactoring the unitary_synthesis.rs code, as well as separating unitary_synthesis.py and the DefaultUnitarySynthesis plugin.
So it seems that the plugin is not being tested, and also behaves differently than the UnitarySynthesis pass?

ShellyGarion · 2025-02-12T09:33:46Z

crates/accelerate/src/unitary_synthesis.rs

+    if euler_basis_set.basis_supported(EulerBasis::U3)
+        && euler_basis_set.basis_supported(EulerBasis::U321)
+    {
+        euler_basis_set.remove(EulerBasis::U3);


what happens if a user provides both 'U' and 'U3'?

They are both included in the target euler_basis_set and given to the 1q decomposer to deal with.

if euler_basis_set.basis_supported(EulerBasis::U3) == true and if euler_basis_set.basis_supported(EulerBasis::U) == true
should we remove one of them?

This part of the code literally follows the logic from the 1q euler decomposer... I am not sure if there was an initial motivation not to remove one of the two (U/U3) or if it was an oversight. I can't think of a reason from the top of my head.

ElePT · 2025-02-12T11:18:38Z

Thanks @ShellyGarion, I guess that the strictly correct thing here would be to duplicate the synthesis unit tests for the plugin. I can do that.

alexanderivrii

Thanks @ElePT, I have carefully reviewed the code and it looks perfect to me, either because it truly is or because I lack the expertise to spot any issues, probably both are true :).

One question (and I have a feeling that I may have already asked it at some point, but then forgot the answer) is why we need some code duplication between rust and python sides, i.e. why can't DefaultSynthesisPlugin just call the relevant functionality implemented in rust? I might be missing something even more basis here, doesn't UnitarySynthesis pass by default call the DefaultSynthesisPlugin?

And special thanks adding code comments, this helped me a lot to understand what each different function does.

I am approving it, but I think there were also some review comments from @ShellyGarion.

…nitary-synth

ElePT · 2025-02-17T12:09:20Z

Thanks @alexanderivrii! To answer your question, the code shouldn't be duplicated. The reason why it is is that at the time the unitary synthesis code was originally oxidized, we didn't have any reference of how to interact with the plugin infrastructure, and because we only had 1 unitary synthesis plugin (the infrastructure wasn't really taken advantage of), we went for the path of replicating the whole flow in rust. In hindsight it would be way more modular to have only the plugin run method in rust, but implementing this now requires a refactoring that I didn't have the time to prioritize yet. Once you finish the HLS oxidization it might also be a good idea to try to make unitary synthesis is pluggable in the HLS workflow. This is something I briefly discussed with @Cryoris. The TL:DR is: not enough information at the time, not enough time to fix it when the information was clear.

ElePT · 2025-02-17T16:38:58Z

Adding the plugin unit tests will probably take a bit more effort than originally planned. If you are fine with merging the PR as-is, I can address the test situation in a follow-up (it's anyways a non user-facing change).

ShellyGarion

LGTM. Thanks!

ElePT mentioned this pull request Jan 21, 2025

Remove use of BackendProperties (BackendV1) in UnitarySynthesis pass #13706

Merged

ElePT added this to the 2.0.0 milestone Jan 21, 2025

ElePT added the on hold Can not fix yet label Jan 21, 2025

ElePT changed the title ~~Oxidize UnitarySynthesis path using basis_gates, remove use of BackendV1 in tests~~ Oxidize UnitarySynthesis path using basis_gates Jan 21, 2025

1ucian0 assigned alexanderivrii Jan 30, 2025

alexanderivrii reviewed Feb 4, 2025

View reviewed changes

1ucian0 assigned ElePT Feb 6, 2025

1ucian0 removed the on hold Can not fix yet label Feb 10, 2025

ElePT force-pushed the revamp-unitary-synth branch from 68740c5 to 20fb3ae Compare February 10, 2025 22:24

Oxidize basis gates path in UnitarySynthesis pass

f9b93cc

ElePT force-pushed the revamp-unitary-synth branch from 20fb3ae to f9b93cc Compare February 10, 2025 22:25

ElePT marked this pull request as ready for review February 10, 2025 22:26

ElePT requested a review from a team as a code owner February 10, 2025 22:26

ElePT added 2 commits February 11, 2025 10:30

Fix lint

08131f3

Rust refactoring: reduce code repetition, reorder file to avoid jumps…

52cf927

… in logic, add comments and docstrings

ElePT force-pushed the revamp-unitary-synth branch from 0200fd2 to 35c24a2 Compare February 11, 2025 12:20

ElePT force-pushed the revamp-unitary-synth branch from 35c24a2 to 7a1914c Compare February 11, 2025 12:31

Fix import path in test

f02fb58

ShellyGarion reviewed Feb 12, 2025

View reviewed changes

alexanderivrii previously approved these changes Feb 12, 2025

View reviewed changes

Merge branch 'main' of https://github.com/Qiskit/qiskit into revamp-u…

58e946e

…nitary-synth

ElePT dismissed alexanderivrii’s stale review via 58e946e February 17, 2025 11:51

ShellyGarion approved these changes Feb 17, 2025

View reviewed changes

ElePT added this pull request to the merge queue Feb 17, 2025

Merged via the queue into Qiskit:main with commit 26906d3 Feb 17, 2025
17 checks passed

ElePT added the Changelog: None Do not include in changelog label Mar 31, 2025

Oxidize UnitarySynthesis path using basis_gates #13704

Oxidize UnitarySynthesis path using basis_gates #13704

Uh oh!

Conversation

ElePT commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

coveralls commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 13369594575

Details

💛 - Coveralls

Uh oh!

alexanderivrii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexanderivrii Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

ElePT Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

1ucian0 commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiskit-bot commented Feb 10, 2025

Uh oh!

ElePT commented Feb 11, 2025

Uh oh!

ShellyGarion left a comment

Choose a reason for hiding this comment

Uh oh!

ShellyGarion Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

ElePT Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

ShellyGarion Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

ElePT Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

ElePT commented Feb 12, 2025

Uh oh!

alexanderivrii left a comment

Choose a reason for hiding this comment

Uh oh!

ElePT commented Feb 17, 2025

Uh oh!

ElePT commented Feb 17, 2025

Uh oh!

ShellyGarion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Oxidize `UnitarySynthesis` path using `basis_gates` #13704

Oxidize `UnitarySynthesis` path using `basis_gates` #13704

ElePT commented Jan 21, 2025 •

edited

Loading

coveralls commented Jan 22, 2025 •

edited

Loading

1ucian0 commented Feb 10, 2025 •

edited

Loading