Consistent identity removal for ConsolidateBlocks#16082
Consistent identity removal for ConsolidateBlocks#16082Cryoris wants to merge 1 commit intoQiskit:mainfrom
ConsolidateBlocks#16082Conversation
Update `ConsolidateBlocks` to also use the average gate fidelity, like `RemoveIdentityEquiv` and the remainder of the stack, to check whether gates are equivalent to the identity.
Cryoris
added
stable backport potential
|
One or more of the following people are relevant to this code:
|
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
Thanks for fixing this. Functionally the change looks good, I just have a few small inline comments.
| use crate::passes::remove_identity_equiv::{MINIMUM_TOL, average_gate_fidelity_below_tol}; | ||
| use crate::passes::unitary_synthesis::{PARAM_SET, TWO_QUBIT_BASIS_SET}; |
There was a problem hiding this comment.
Neither here nor there but you could simplify this a bit to:
| use crate::passes::remove_identity_equiv::{MINIMUM_TOL, average_gate_fidelity_below_tol}; | |
| use crate::passes::unitary_synthesis::{PARAM_SET, TWO_QUBIT_BASIS_SET}; | |
| use super::remove_identity_equiv::{MINIMUM_TOL, average_gate_fidelity_below_tol}; | |
| use super::unitary_synthesis::{PARAM_SET, TWO_QUBIT_BASIS_SET}; |
| super().__init__() | ||
| self.basis_gates = None | ||
| self.basis_gate_name = None | ||
| self.approximation_degree = approximation_degree |
There was a problem hiding this comment.
Just for good measure even though it's not documented so not public I'd still make this
| self.approximation_degree = approximation_degree | |
| self._approximation_degree = approximation_degree |
just so it's abundantly clear.
| fixes: | ||
| - | | ||
| Fixed an inconsistency in :class:`.ConsolidateBlocks`, which removed identity-equivalent | ||
| sequences differently to how :class:`.RemoveIdentityEquiv` and the remainder of the stack does. |
There was a problem hiding this comment.
| sequences differently to how :class:`.RemoveIdentityEquiv` and the remainder of the stack does. | |
| sequences differently to how :class:`.RemoveIdentityEquiv` and Qiskit's other transpiler passes. |
| - | | ||
| Fixed an inconsistency in :class:`.ConsolidateBlocks`, which removed identity-equivalent | ||
| sequences differently to how :class:`.RemoveIdentityEquiv` and the remainder of the stack does. | ||
| This has been fixed and these passes now behave consistently by using the average gate |
There was a problem hiding this comment.
| This has been fixed and these passes now behave consistently by using the average gate | |
| This has been fixed and :class:`.ConsolidateBlocks` now behaves consistently by using the average gate |
| sequences differently to how :class:`.RemoveIdentityEquiv` and the remainder of the stack does. | ||
| This has been fixed and these passes now behave consistently by using the average gate | ||
| fidelity to check whether a sequence is close to the identity. This implies that also | ||
| sequences that are equivalent to the identity up to global phase are now being removed. |
There was a problem hiding this comment.
Functionally this would happen later anyway when we run UnitarySynthesis, so it only really changes the behavior of the pass in isolation.
| qc.cx(0, 1) | ||
| qc.h(0) | ||
|
|
||
| pm = PassManager([Collect2qBlocks(), ConsolidateBlocks()]) |
There was a problem hiding this comment.
You technically don't need the Collect2qBlocks call anymore. If you the pass is run without the blocks list in the property set it runs the analysis itself as the initial step.
| // if the performance of this pass changes over time. | ||
| const PARALLEL_THRESHOLD: usize = 50_000; | ||
|
|
||
| pub const MINIMUM_TOL: f64 = 1e-12; |
There was a problem hiding this comment.
Not critical, but does this need to public outside of transpile or would pub(crate) or pub(super) be sufficient?
mtreinish
removed
the
stable backport potential
| pm = PassManager([Collect2qBlocks(), ConsolidateBlocks()]) | ||
| self.assertEqual(QuantumCircuit(5), pm.run(qc)) | ||
|
|
||
| def test_identity_unitary_is_removed_up_to_phase(self): |
There was a problem hiding this comment.
does it's also worth checking the approximation_degree parameter?
namely, that an angle close to 2pi is not removed if the tolerance is high enough?
4 participants
Update
ConsolidateBlocksto also use the average gate fidelity, likeRemoveIdentityEquivand the remainder of the stack, to check whether gates are equivalent to the identity.AI/LLM disclosure