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

Composing circuit with a switch statement causes a bug in the QASM3 exporter #10108

Closed
taalexander opened this issue May 13, 2023 · 4 comments · Fixed by #10164
Closed

Composing circuit with a switch statement causes a bug in the QASM3 exporter #10108

taalexander opened this issue May 13, 2023 · 4 comments · Fixed by #10164
Assignees
Labels
bug Something isn't working
Milestone

Comments

@taalexander
Copy link
Contributor

Environment

  • Qiskit Terra version: 0.25.0
  • Python version: 3.9
  • Operating system: OSX

What is happening?

When composing a circuit with another circuit containing a switch statement and then exporting to OpenQASM 3. I receive an error

KeyError: "'ClassicalRegister(3, 'random')' is not defined in the current context"

This may be related to #9746.

How can we reproduce the issue?

Run

from qiskit import QuantumCircuit, QuantumRegister, ClassicalRegister

qr = QuantumRegister(3)
cr_random = ClassicalRegister(3, "random")
cr_result = ClassicalRegister(3, "result")
qc = QuantumCircuit(qr, cr_random, cr_result)

# build a random state
qc.h(qr)
# measure the state
qc.measure(qr, cr_random)

with qc.switch(cr_random) as case:
    with case(1):
        qc.x(qr[0])
    with case(2):
        qc.x(qr[1])
    with case(3):
        qc.x(qr[0])
        qc.x(qr[1])
    with case(4):
        qc.x(qr[2])
    with case(5):
        qc.x(qr[0])
        qc.x(qr[2])
    with case(6):
        qc.x(qr[1])
        qc.x(qr[2])
    with case(7):
        qc.x(qr)
    with case(case.DEFAULT):
        pass

qc.measure(qr, cr_result)

composed = QuantumCircuit(3, 6)
composed.compose(qc, inplace=True)

from qiskit import qasm3
print(qc_qasm3 := qasm3.dumps(composed, disable_constants=True, experimental=qasm3.ExperimentalFeatures.SWITCH_CASE_V1))

What should happen?

QASM3 should be exported.

Any suggestions?

No response

@taalexander taalexander added the bug Something isn't working label May 13, 2023
@taalexander
Copy link
Contributor Author

I have verified that using append also does not work and results in the same error.

@dieris
Copy link
Contributor

dieris commented May 15, 2023

I believe what needs to happen is to remap the ClassicalRegister in the SwitchCaseOp target analogously to the condition in the IfElseOp here

@mtreinish mtreinish added this to the 0.24.1 milestone May 15, 2023
@dieris
Copy link
Contributor

dieris commented May 24, 2023

I believe what needs to happen is to remap the ClassicalRegister in the SwitchCaseOp target analogously to the condition in the IfElseOp here

this may be obvious, but I just realized that this only addresses the issue with compose. For append to work, a dual fix is also needed for the DAGCircuit in substitute_node_with_dag to map the target cbits when the operation has only a target instead of a condition.

@jakelishman
Copy link
Member

The surface-level problem in compose is not too difficult to fix, but that alone won't be enough in the current form of qasm3-runner to make things work. There's further demons lurking in nested control flow, I'm fairly sure, but that's a further issue. For the time being in append, I think circuit_to_instruction will need to error out if there are ever any classical registers that are used; there's no current way for this to properly make sense as we've got no concept of scoping a classical value.

If I were to patch compose to handle the simplest case, the code as given would then fail in the OQ3 exporter. It's not immediately clear to me how we could / if we should fix that entirely from the Terra side in a way that the QSS compiler can ingest. The initial creation of composed contains one ClassicalRegister(6, name="c"), so when we compose the circuit with two registers onto it, we're going to have overlap between c (in composed) and cr_random (in qc). Qiskit will happily accept this (it's valid in our model), but I don't think qss-compiler supports the OQ3 aliasing semantics needed to represent it.

As a temporary work-around at least, the base of the composition (composed, in this case) could be created as

from qiskit.circuit import QuantumCircuit, Qubit, Clbit

base = QuantumCircuit([Qubit() for _ in [None]*n_qubits], [Clbit() for _ in [None]*n_clbits])

(see #6496 for an issue that's proposing that these semantics should be the default for QuantumCircuit(int, int), but that would be a breaking change in Terra so it can't happen overnight). That may be sufficient for your use cases, though it doesn't fully solve everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants