-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Only apply MCMT plugin on MCMTGate
#13596
base: main
Are you sure you want to change the base?
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 12465784214Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks straightforward, LGTM!
I had one tiny off-topic question.
mcmt = MCMT(gate=gate, num_ctrl_qubits=1, num_target_qubits=1) | ||
circuit.append(mcmt, circuit.qubits) # append the MCMT circuit as gate called "MCMT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit off-topic. Here we are appending the quantum circuit mcmt
to the quantum circuit circuit
, yet the docstring for append
states instruction: Operation | CircuitInstruction
. The code does work, since a QuantumCircuit
defines a to_instruction
method, which append
tries to use when the instruction is not of type Operation
. Would it be slightly cleaner to change to circuit.append(mcmt.to_instruction(), ...)
, or should we change the typehint for the append
method?
Summary
Fixes #13563, by enabling the high-level synthesis plugins of MCMT only for the
MCMTGate
, not any gate called"mcmt"
.Details and comments
While we often let it be the user's responsibility to be mindful with gate names, this is a special case as we provide a circuit called
"mcmt"
. This can cause innocent-looking code to break, so we should add this check at least until theMCMT
circuit is removed 🙂