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

fix missing inverse definitions in generate_basis_approximations #13517

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

burgholzer
Copy link
Contributor

Summary

This tiny PR fixes an oversight in the generation routine for basis approximations that is used in the SolovayKitaev synthesis pass.

Details and comments

While the dictionary of single-qubit gates listed the sx and sxdg gates, the dictionary of inverses was missing those definitions.
This PR simply adds the respective definitions.

Should this receive a release note?

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Dec 2, 2024
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Dec 2, 2024

Pull Request Test Coverage Report for Build 12233529353

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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 17 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.007%) to 88.93%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 1 92.98%
qiskit/circuit/add_control.py 4 96.49%
crates/qasm2/src/parse.rs 12 96.69%
Totals Coverage Status
Change from base Build 12122746645: -0.007%
Covered Lines: 79352
Relevant Lines: 89230

💛 - Coveralls

@Cryoris
Copy link
Contributor

Cryoris commented Dec 3, 2024

Nice catch! Did you notice this because you saw decompositions with SX-SXdg sequences, which should've been cancelled? If yes, it would be nice if we could add a regression test that ensures this behavior is now fixed.

@burgholzer
Copy link
Contributor Author

Nice catch! Did you notice this because you saw decompositions with SX-SXdg sequences, which should've been cancelled? If yes, it would be nice if we could add a regression test that ensures this behavior is now fixed.

Not really. I was trying to set up a transpilation pipeline for compiling arbitrary circuits to the Clifford+T gate-set. I wanted to throw all possible single-qubit Clifford gates into the basis approximations method, including the sx and sxdg gates. Doing so made the function error out.
I believe this was missed as it was not tested in the existing unit test. The addition in 4e8b90d should make sure this works as expected.
Do you believe additional tests are necessary here?

@Cryoris
Copy link
Contributor

Cryoris commented Dec 4, 2024

Ah so the test you added fails without sx and sxdg in the _1q_inverses dictionary? If yes, then we don't need any further tests 🙂 However, as you suggested above, this merits a bugfix release note.

@Cryoris Cryoris added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Dec 4, 2024
@Cryoris Cryoris added this to the 1.4.0 milestone Dec 4, 2024
@Cryoris Cryoris added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Dec 4, 2024
@burgholzer
Copy link
Contributor Author

Ah so the test you added fails without sx and sxdg in the _1q_inverses dictionary? If yes, then we don't need any further tests 🙂 However, as you suggested above, this merits a bugfix release note.

Yeah, exactly.
Added a release note just now. Sorry that took so long... these days are quite busy.
Let me know if you want anything changed 😌

@alexanderivrii
Copy link
Contributor

Hi @burgholzer, a question not directly related to the PR: what is the best way to transpile into Clifford+T (i.e. how did you set up the transpilation pipeline for that)? Thx!

@burgholzer
Copy link
Contributor Author

Hi @burgholzer, a question not directly related to the PR: what is the best way to transpile into Clifford+T (i.e. how did you set up the transpilation pipeline for that)? Thx!

Hi 👋🏼
That's a good question. Right now, the respective code is still a bit messy and not as clean as I would have liked it to be.

from qiskit import transpile
from qiskit.converters import circuit_to_dag, dag_to_circuit
from qiskit.transpiler.passes.synthesis import SolovayKitaev

# The quantum circuit to transpile
qc = ...

# Clifford+T gateset
gateset = [
    "i",
    "x",
    "y",
    "z",
    "h",
    "s",
    "sdg",
    "t",
    "tdg",
    "sx",
    "sxdg",
    "cx",
    "cy",
    "cz",
    "swap",
    "iswap",
    "dcx",
    "ecr",
    "measure",
    "barrier",
]

# Transpile the circuit to single- and two-qubit gates including rotations
compiled_for_sk = transpile(
    qc,
    basis_gates=[*gateset, "rx", "ry", "rz"],
)
# Synthesize the rotations to Clifford+T gates
# Measurements are removed and added back after the synthesis to avoid errors in the Solovay-Kitaev pass
pass_ = SolovayKitaev()
new_qc = dag_to_circuit(pass_.run(circuit_to_dag(compiled_for_sk.remove_final_measurements(inplace=False))))
new_qc.measure_all()
# Transpile once more to remove unnecessary gates and optimize the circuit
compiled_qc = transpile(
    new_qc,
    basis_gates=gateset,
)

While one could use the "sk" unitary synthesis method with the transpile command, I could not make that work in any combination I tried, as it would simply not transpile the circuit to the respective gate-set. So something like

from qiskit import QuantumCircuit, transpile

import numpy as np

qc = QuantumCircuit(2)
qc.cp(np.pi/4, 0, 1)

gateset = [
    "i",
    "x",
    "y",
    "z",
    "h",
    "s",
    "sdg",
    "t",
    "tdg",
    "sx",
    "sxdg",
    "cx",
    "cy",
    "cz",
    "swap",
    "iswap",
    "dcx",
    "ecr",
    "measure",
    "barrier",
]

compiled_qc = transpile(
    qc,
    basis_gates=gateset,
    unitary_synthesis_method = "sk",
)

would not work an error out because the synthesis does nothing, and then the basis gate decomposer cannot find a decomposition into the discrete basis.

However, I haven't had the time to fully work this out and report it as an issue.

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 Community PR PRs from contributors that are not 'members' of the Qiskit repo stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants