-
Notifications
You must be signed in to change notification settings - Fork 25
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
catch all multi target uncontrolled gates #214
Conversation
@@ -244,7 +236,7 @@ def qiskit_circ_to_ionq_circ( | |||
converted["gate"] = instruction_name | |||
|
|||
# Make sure uncontrolled multi-targets use all qargs. | |||
if isinstance(instruction, multi_target_uncontrolled_gates): | |||
if instruction.num_qubits > 1 and not hasattr(instruction, "num_ctrl_qubits"): |
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.
is num_qubits guaranteed to be set?
I don't quite understand why we are changing this and not
qiskit-ionq/qiskit_ionq/helpers.py
Lines 193 to 194 in 08a15dc
if instruction_name not in GATESET_MAP[gateset]: | |
raise ionq_exceptions.IonQGateError(instruction_name, gateset) |
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.
Spoke offline: main issue is that where I linked we check gate name, and here we are checking python classes, which might not match in the case of QASM/etc
WDYT about changing to the values from GATESET_MAP that we know should have multiple controls?
This LGTM assuming the num_qubits attribute must exist, if it might not I would change to getattr(instruction, "num_qubits", 0)
@@ -244,7 +236,7 @@ def qiskit_circ_to_ionq_circ( | |||
converted["gate"] = instruction_name | |||
|
|||
# Make sure uncontrolled multi-targets use all qargs. | |||
if isinstance(instruction, multi_target_uncontrolled_gates): | |||
if instruction.num_qubits > 1 and not hasattr(instruction, "num_ctrl_qubits"): |
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.
Spoke offline: main issue is that where I linked we check gate name, and here we are checking python classes, which might not match in the case of QASM/etc
WDYT about changing to the values from GATESET_MAP that we know should have multiple controls?
This LGTM assuming the num_qubits attribute must exist, if it might not I would change to getattr(instruction, "num_qubits", 0)
Summary
instead of checking against a tuple, check the quality of the gate
Details and comments
if the gate has multiple targets and no control qubits