Correctly updating global phase when removing gates that are identity up to a global phase #13785
Conversation
alexanderivrii
added
stable backport potential
|
One or more of the following people are relevant to this code:
|
|
Update: there is another part of the pass dealing with RX, RY, RZ and RXXGate, RYYGate, RZZGate, RZXGate gates |
Pull Request Test Coverage Report for Build 13242973242Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Thanks for tackling this!
I would expect this needs to cover the full spectrum of operations that are the identity up to a global phase, i.e. exp(i phi) * I 🙂 This could be done e.g. by just looking at the first entry of the matrix and extracting the phase (similar to how it is done in Operator.equiv).
|
@Cryoris, indeed, you are absolutely correct, we have the same problem with all of the operations that are identity up to a global phase, I will need to check in an updated fix shortly. A related question is do you know why the global phase gates were ignored by RemoveIdentityEquivalent pass? |
alexanderivrii
changed the title
gates that are equivalent to identity up to a global phase
|
Hmm, after merging with main (and in particular with #13759), I had to do a small fix to support unitary matrices coming from unitary gates. However, this means that the pass now can't be backported as is, in the backported version the block for unitary matrices needs to be removed. |
There was a problem hiding this comment.
LGTM. Thanks @alexanderivrii for fixing this bug. I'm not sure to which branch it should be merged.
| // Skip global phase like gate | ||
| if gate.num_qubits() < 1 { | ||
| continue; | ||
| if let Some(matrix) = gate.matrix(inst.params_view()) { |
There was a problem hiding this comment.
I wonder if the Gate and UnitaryGate branches could be merged, as they are identical (right?) and both are OperationRef and provide the matrix method... maybe something like
let view = inst.op.view();
match view {
// existing parts...
OperationRef::Gate | OperationRef::Unitary => {
if let Some(matrix) = view.matrix(inst.params_view()) {
// existing parts...
}
}
}
There was a problem hiding this comment.
This is a nice suggestion, but the compiler does not like it. The problem is that gate has different types in the two arms: in OperationRef::Gate(gate) it's a &PyGate, and in OperationRef::UnitaryGate(gate) it's a &UnitaryGate.
|
|
||
| if global_phase_update != 0. { | ||
| dag.add_global_phase(py, &Param::Float(global_phase_update)) | ||
| .unwrap(); |
There was a problem hiding this comment.
The addition can only fail if the Parameter is not a float, so we can give a reason why this shouldn't fail (and if it does fail, then we know what assumption we had that's wrong 🙂)
| .unwrap(); | |
| .unwrap("The global phase is guaranteed to be a float"); |
There was a problem hiding this comment.
Done in 68e6a7f. And of course, you mean expect instead of unwrap.
| Fixed a bug in :class:`.RemoveIdentityEquivalent` transpiler pass, where the | ||
| unitary gates close to identity up to a global phase were removed from the circuit, | ||
| but the global phase of the circuit was not updated. In particular, | ||
| :class:`.RemoveIdentityEquivalent` now removes non-parameterized :class:`.GlobalPhaseGate` | ||
| gates. |
There was a problem hiding this comment.
To me the adjective "unitary" here could mean we refer to UnitaryGate objects, maybe removing it is better (since it could have been any other gate)?
| Fixed a bug in :class:`.RemoveIdentityEquivalent` transpiler pass, where the | |
| unitary gates close to identity up to a global phase were removed from the circuit, | |
| but the global phase of the circuit was not updated. In particular, | |
| :class:`.RemoveIdentityEquivalent` now removes non-parameterized :class:`.GlobalPhaseGate` | |
| gates. | |
| Fixed a bug in the :class:`.RemoveIdentityEquivalent` transpiler pass, where | |
| gates close to identity up to a global phase were removed from the circuit, | |
| but the global phase of the circuit was not updated. In particular, | |
| :class:`.RemoveIdentityEquivalent` now removes non-parameterized :class:`.GlobalPhaseGate` | |
| gates. |
There was a problem hiding this comment.
Sure, done in 68e6a7f.
Summary
This fixes a bug in the$e^{i\theta}$ , 0], [0, $e^{i\theta}$ ]] for fix floating-point values of $\theta$ , standard gates $R_X(2\pi)$ , $R_Y(2\pi)$ , $R_Z(2\pi)$ (whose matrices are close to $-I$ ), and some others. Now the pass removes these gates and correctly updates the global phase of the circuit. In particular, the pass now handles non-parameterized
RemoveIdentityEquivalenttranspiler pass (the pass was introduced in #12384), where unitary gates close to identity up to a global phase were removed from the circuit but the global phase of the circuit was not updated. This includes, for instance, single-qubit unitary gates with matrices close to [[GlobalPhaseGatesas well.Fixes #13778.
Details and comments:
The pass ignores parameterized gates. We could probably relatively easily extend it to handle parameterized global phase gates.
The thing to notice in the current documentation of the pass is that the current condition for removing a gate is indeed satisfied for gates of the form$e^{i\theta} I$ . And in fact the opposite is also true (thanks to @ShellyGarion for helping me work out the math): if the removal condition is satisfied, then a gate is necessarily of the form $e^{i\theta} I$ .
Lol, yet another change after the #13759 was merged is to now also explicitly handle Unitary gates.
Thanks to @Cryoris and @ShellyGarion for joint debugging of the problem.