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

Overloaded gate names cause panic in commutation checker #13988

Closed
mtreinish opened this issue Mar 11, 2025 · 0 comments · Fixed by #13991
Closed

Overloaded gate names cause panic in commutation checker #13988

mtreinish opened this issue Mar 11, 2025 · 0 comments · Fixed by #13991
Assignees
Labels
bug Something isn't working
Milestone

Comments

@mtreinish
Copy link
Member

Environment

  • Qiskit version: 2.0.0rc1
  • Python version: 3.13
  • Operating system: Linux

What is happening?

When calling the commutation checker (via CommutativeCancellation typically) with a custom gate that uses a standard gate name that is in the set of specially handled rotation gates names for the commutation checker, but is not a standard gate instance this results in a panic with the message: "Supported gates are standard gates". The root cause is the rust code of the commutation checker is assuming if it gets a standard gate name for a gate in the circuit than it must be a standard gate and when it isn't it panics.

While it's true we make it explicit that the name is like the op code and the standard gates are reserved, this can come up not by user error necessarily, but because of #10737 when loading a qasm file for a gate that's in Qiskit's standard library but not qasm's. So we should be able to gracefully handle this case correctly by being resilient to this mismatch.

How can we reproduce the issue?

from qiskit.circuit import QuantumCircuit
from qiskit.transpiler import generate_preset_pass_manager, Target, CouplingMap

qasm_str = """OPENQASM 2.0;
include "qelib1.inc";
gate ryy(param0) q0,q1
{
 rx(pi/2) q0;
 rx(pi/2) q1;
 cx q0,q1;
 rz(0.37801308) q1;
 cx q0,q1;
 rx(-pi/2) q0;
 rx(-pi/2) q1;
}
qreg q0[2];
creg c0[2];
h q0[0];
ryy(1.2182379) q0[0],q0[1];
measure q0[0] -> c0[0];
measure q0[1] -> c0[1];
"""

qc = QuantumCircuit.from_qasm_str(qasm_str)
print(qc)


target = Target.from_configuration(["r", "cz", "measure"], num_qubits=5, coupling_map=CouplingMap.from_line(5))
pm = generate_preset_pass_manager(target)
print(pm.run(qc))

What should happen?

The code should not panic, the custom ryy gate resulting from the qasm loading should be either consistently be interpreted as the standard gate or not.

Any suggestions?

I actually caught this in my code review of the most recent change to the logic here: #13874 (review) but didn't think it would result in a panic/error and was just inefficient, I didn't consider overloaded gate names. We can do as I suggested though and use the rust StandardGate type to avoid the panic and handle it consistently in the commutation checker.

@mtreinish mtreinish added the bug Something isn't working label Mar 11, 2025
@mtreinish mtreinish added this to the 2.0.0 milestone Mar 11, 2025
@mtreinish mtreinish self-assigned this Mar 11, 2025
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Mar 11, 2025
This commit reworks the internals of the CommutationChecker to not rely
on operation name except for where we do a lookup by name in the
commutation library provided (which is the only key available to support
custom gates). This fixes the case where a custom gate that overloads
the standard gate name, previously the code would assume it to be a
standard gate and internally panic when it wasn't. When working with
standard gates (or standard instructions) we don't need to rely on
string matching because we can rely on the rust data model to do the
heavy lifting for us. This commit moves all the explicit handling of
standard gates to use the StandardGate type directly and makes this
logic more robust.

This also removes are usage of the once_cell library in
qiskit-accelerate because it was used to create a lazy static hashsets
of strings which are no longer needed because static lookup tables replace
this when we stopped using string comparisons.

Fixes Qiskit#13988
github-merge-queue bot pushed a commit that referenced this issue Mar 12, 2025
* Fix string and standard gate mismatch in commutation checker

This commit reworks the internals of the CommutationChecker to not rely
on operation name except for where we do a lookup by name in the
commutation library provided (which is the only key available to support
custom gates). This fixes the case where a custom gate that overloads
the standard gate name, previously the code would assume it to be a
standard gate and internally panic when it wasn't. When working with
standard gates (or standard instructions) we don't need to rely on
string matching because we can rely on the rust data model to do the
heavy lifting for us. This commit moves all the explicit handling of
standard gates to use the StandardGate type directly and makes this
logic more robust.

This also removes are usage of the once_cell library in
qiskit-accelerate because it was used to create a lazy static hashsets
of strings which are no longer needed because static lookup tables replace
this when we stopped using string comparisons.

Fixes #13988

* Rename is_commutation_skipped() is_parameterized()
mergify bot pushed a commit that referenced this issue Mar 12, 2025
* Fix string and standard gate mismatch in commutation checker

This commit reworks the internals of the CommutationChecker to not rely
on operation name except for where we do a lookup by name in the
commutation library provided (which is the only key available to support
custom gates). This fixes the case where a custom gate that overloads
the standard gate name, previously the code would assume it to be a
standard gate and internally panic when it wasn't. When working with
standard gates (or standard instructions) we don't need to rely on
string matching because we can rely on the rust data model to do the
heavy lifting for us. This commit moves all the explicit handling of
standard gates to use the StandardGate type directly and makes this
logic more robust.

This also removes are usage of the once_cell library in
qiskit-accelerate because it was used to create a lazy static hashsets
of strings which are no longer needed because static lookup tables replace
this when we stopped using string comparisons.

Fixes #13988

* Rename is_commutation_skipped() is_parameterized()

(cherry picked from commit abb0cf9)

# Conflicts:
#	crates/accelerate/src/commutation_checker.rs
github-merge-queue bot pushed a commit that referenced this issue Mar 12, 2025
…#13991) (#14004)

* Fix string and standard gate mismatch in commutation checker (#13991)

* Fix string and standard gate mismatch in commutation checker

This commit reworks the internals of the CommutationChecker to not rely
on operation name except for where we do a lookup by name in the
commutation library provided (which is the only key available to support
custom gates). This fixes the case where a custom gate that overloads
the standard gate name, previously the code would assume it to be a
standard gate and internally panic when it wasn't. When working with
standard gates (or standard instructions) we don't need to rely on
string matching because we can rely on the rust data model to do the
heavy lifting for us. This commit moves all the explicit handling of
standard gates to use the StandardGate type directly and makes this
logic more robust.

This also removes are usage of the once_cell library in
qiskit-accelerate because it was used to create a lazy static hashsets
of strings which are no longer needed because static lookup tables replace
this when we stopped using string comparisons.

Fixes #13988

* Rename is_commutation_skipped() is_parameterized()

(cherry picked from commit abb0cf9)

# Conflicts:
#	crates/accelerate/src/commutation_checker.rs

* Update commutation_checker.rs

---------

Co-authored-by: Matthew Treinish <[email protected]>
Co-authored-by: Elena Peña Tapia <[email protected]>
raynelfss pushed a commit to raynelfss/qiskit that referenced this issue Mar 20, 2025
…13991)

* Fix string and standard gate mismatch in commutation checker

This commit reworks the internals of the CommutationChecker to not rely
on operation name except for where we do a lookup by name in the
commutation library provided (which is the only key available to support
custom gates). This fixes the case where a custom gate that overloads
the standard gate name, previously the code would assume it to be a
standard gate and internally panic when it wasn't. When working with
standard gates (or standard instructions) we don't need to rely on
string matching because we can rely on the rust data model to do the
heavy lifting for us. This commit moves all the explicit handling of
standard gates to use the StandardGate type directly and makes this
logic more robust.

This also removes are usage of the once_cell library in
qiskit-accelerate because it was used to create a lazy static hashsets
of strings which are no longer needed because static lookup tables replace
this when we stopped using string comparisons.

Fixes Qiskit#13988

* Rename is_commutation_skipped() is_parameterized()
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.

1 participant