Skip to content
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

Oxidize UnitarySynthesis path using basis_gates #13704

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Jan 21, 2025

Summary

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

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

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

@ElePT ElePT added this to the 2.0.0 milestone Jan 21, 2025
@ElePT ElePT added the on hold Can not fix yet label Jan 21, 2025
@ElePT 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
@coveralls
Copy link

Pull Request Test Coverage Report for Build 12905680507

Details

  • 178 of 194 (91.75%) changed or added relevant lines in 2 files are covered.
  • 81 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.06%) to 88.882%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/unitary_synthesis.rs 169 185 91.35%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/two_qubit_decompose.rs 1 92.12%
crates/accelerate/src/unitary_synthesis.rs 1 93.25%
crates/circuit/src/dag_node.rs 3 79.23%
crates/qasm2/src/lex.rs 4 92.23%
crates/qasm2/src/parse.rs 12 97.15%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 60 46.68%
Totals Coverage Status
Change from base Build 12905478077: -0.06%
Covered Lines: 79434
Relevant Lines: 89370

💛 - Coveralls

Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

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.

}

// Used in get_2q_decomposers. If the found 2q basis is a subset of GOODBYE_SET,
// then we know TwoQubitBasisDecomposer is an ideal decomposition and there is
// no need to bother trying the XXDecomposer.
static GOODBYE_SET: [&str; 3] = ["cx", "cz", "ecr"];

fn get_euler_basis_set(basis_list: IndexSet<&str>) -> EulerBasisSet {
Copy link
Contributor

Choose a reason for hiding this comment

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

A small question about the type of basis_list: why does it need to be a set (and not say a vec) and why does it need to be index (the order is not important here, right?)

)?;
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Some(params.iter().map(|p| Param::Float(*p)).collect()),
inst_qubits,
),
}
});
let synth_error_from_target = synth_error(py, scoring_info, target);
let synth_error_from_target = synth_error(py, scoring_info, target.unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why target can't be None here?

Comment on lines -353 to -354
backend_props (BackendProperties): Properties of a backend to
synthesize for (e.g. gate fidelities).
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see, the BackendProperties should be removed by the "prequel" PR #13706, correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold Can not fix yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants