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

Ignoring unsupported gates for Solovay-Kitaev transpiler pass #13690

Merged
merged 4 commits into from
Feb 18, 2025

Conversation

alexanderivrii
Copy link
Member

Summary

The Solovay-Kitaev transpiler pass (i.e. the standalone pass and not the unitary synthesis plugin) previously considered all operations over $1$ qubit and ignored all operations over $k$ qubits for $k\ne 1$. In doing so, it raised a transpiler error for circuits with single-qubit operations that are either parameterized or don't have a to_matrix method (these include barriers, measures, control-flow ops and more). This PR changes the default behavior to ignore such operations instead of raising an error.

@alexanderivrii alexanderivrii requested review from ShellyGarion and a team as code owners January 19, 2025 15:48
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@alexanderivrii alexanderivrii added this to the 2.0.0 milestone Jan 19, 2025
@coveralls
Copy link

coveralls commented Jan 19, 2025

Pull Request Test Coverage Report for Build 13070354287

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

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1488 unchanged lines in 47 files lost coverage.
  • Overall coverage decreased (-0.1%) to 88.814%

Files with Coverage Reduction New Missed Lines %
crates/circuit/src/lib.rs 1 94.64%
crates/accelerate/src/basis/basis_translator/basis_search.rs 1 99.3%
qiskit/synthesis/linear/linear_depth_lnn.py 1 90.91%
crates/qasm3/src/build.rs 1 72.24%
crates/accelerate/src/isometry.rs 1 99.65%
crates/accelerate/src/gate_direction.rs 2 97.58%
crates/accelerate/src/unitary_synthesis.rs 2 93.09%
crates/qasm3/src/lib.rs 2 87.88%
qiskit/dagcircuit/collect_blocks.py 2 98.34%
qiskit/primitives/containers/bit_array.py 2 96.32%
Totals Coverage Status
Change from base Build 12835664036: -0.1%
Covered Lines: 79759
Relevant Lines: 89805

💛 - Coveralls

@Cryoris
Copy link
Contributor

Cryoris commented Jan 20, 2025

I agree it should leave Measure and Reset as is, but I'm not sure if we should just ignore gates that don't have to_matrix. I always thought of SK like a kind of basis translator that takes you to the target 1q basis and, like BasisTranslator, it would fail if it can't 🤔

@alexanderivrii
Copy link
Member Author

I agree it should leave Measure and Reset as is, but I'm not sure if we should just ignore gates that don't have to_matrix. I always thought of SK like a kind of basis translator that takes you to the target 1q basis and, like BasisTranslator, it would fail if it can't 🤔

This is indeed a valid alternative. I like the analogy with the BasisTranslator pass, though (unlike BT) SK already ignores all operations on 2 or more qubits, and it feels strange that the pass should ignore, for instance, the 2-qubit initialize instruction while throw an error for the 1-qubit initialize instruction.

The other thing is that probably we need to make the pass recursive for control-flow ops (and not just ignore these as it happens right now), what do you think?

Maybe we should first decide on the typical usages of the SK pass, when and how it would be used in a typical transpiler pipeline.

@jakelishman
Copy link
Member

jakelishman commented Jan 20, 2025

The SK algorithm only applies to decompositions of $U(2)$, so it makes sense to ignore anything that isn't in that. Under that rule, ignoring both Measure and any Initialize is consistent, because neither is unitary.

Agree that the pass should recurse over control-flow operations.

If we're intending to use the SK algorithm as part of a translation stage, we might want to consider it as an entire pipeline. To use the BasisTranslator example: that alone isn't an entire translation stage; the stage also includes Unroll3qOrMore. If we want to make an SK-based translation plugin, I'd suggest we make an entire plugin, rather than trying to make the pass try to guess - the pipeline can be something like

  • unroll 3q or more
  • consolidate blocks
  • unitary synthesis (min_qubits=2) to a target basis [u, <2q gates>]
  • SK

In an ideal world, we'd want UnitarySynthesis to output the 1q unitaries without bothering to find their Euler-angle decomposition, but we could potentially work out how to improve that story later.

I wouldn't necessarily make the SK pass error out if it can't translate, since it's conceivable to me that you might want to use it in other contexts - we can make the translation stage error out in that case, potentially, where it's clear that the stage is required to fail if translation can't be done.

@alexanderivrii
Copy link
Member Author

alexanderivrii commented Jan 22, 2025

Thanks @jakelishman, @Cryoris! I have added recursion for control-flow operators. If we agree that the SK pass should not error out if it can't translate, is there more you suggest to do here, modulo correcting the class documentation and polishing the release notes?

Let's take the discussion of the transpilation pipeline to #13695.

…yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Copy link
Contributor

@Cryoris Cryoris 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!

@Cryoris Cryoris added this pull request to the merge queue Feb 18, 2025
Merged via the queue into Qiskit:main with commit 97ed168 Feb 18, 2025
17 checks passed
@ElePT ElePT added the Changelog: API Change Include in the "Changed" section of the changelog label Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

6 participants