Fix string and standard gate mismatch in commutation checker #13991
Conversation
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
mtreinish
added
stable backport potential
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13808858937Details
💛 - Coveralls |
There was a problem hiding this comment.
That's more elegant plus also correct 😛 I left some minor comments below.
Should we add an upgrade reno stating the change in behavior that custom gates matching standard gate names will no longer be fast-tracked through the commutation checker? I'm imaging a scenario where someone creates a custom gate named e.g. "ryy" and doesn't give it a matrix -- then it would've worked before 2.0 but doesn't anymore now, right?
| } | ||
| } | ||
|
|
||
| if matches!( |
There was a problem hiding this comment.
This is now a logical change since it will eagerly return false on instructions, which is fine here since we don't check commutations between directives and these don't have defined matrices. Should we also check for OperationRef::Operation here, which also doesn't have a matrix and is not in the commutation library (it isn't right?)?
There was a problem hiding this comment.
I didn't view it as a logical change, just a small efficiency change while I was removing string usage, because we were matching a bit too restrictively before here. We know that no Instruction either standard (like measure, reset, etc) or from python (like Intialize). Since as you say we can't have a commutation any instruction as they're explicitly non-unitary.
I left Operation because I thought they could have a matrix (like Clifford) but looking at the operation trait for PyOperation we have it hard coded to None
qiskit/crates/circuit/src/operations.rs
Lines 2726 to 2728 in 6f317f9
Clifford though
There was a problem hiding this comment.
Hmm yes I based this off the Rust implementation for when operations always return None... but I'm fine keeping the code logically correct as is, since we can add operations that have matrices 👍🏻
(Otherwise it would be very easy to introduce a bug if we forget to remove the Operation case here once we add e.g. Cliffords to Rust)
I don't think it's necessary in this case, it's not really supported to have a non-standard gate reuse a name like this. Its only the qasm path which make me think we should fix this. There is no API guarantee that the commutation checker will fast track anything either, as long as it's correct output that's fine. In the case of a custom "ryy" gate we should fall back to matrix based approach (assuming it has a definition or explicit matrix set) so it should all work as documented. |
* 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
…#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]>
…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()
Summary
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
Details and comments