Consistent identity removal for ConsolidateBlocks

crates/transpiler/src/passes/consolidate_blocks.rs

@@ -12,7 +12,7 @@
 
 use super::optimize_1q_gates_decomposition::matmul_1q;
 use hashbrown::{HashMap, HashSet};
-use nalgebra::{Matrix2, Matrix4, U4};
+use nalgebra::{Matrix2, U4};
 use ndarray::ArrayView2;
 use ndarray::{Array2, aview2};
 use num_complex::Complex64;
@@ -42,33 +42,11 @@ use qiskit_synthesis::two_qubit_decompose::{
 use rustworkx_core::petgraph::stable_graph::NodeIndex;
 use smallvec::SmallVec;
 
+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 crate::target::{Qargs, Target};
 use qiskit_circuit::PhysicalQubit;
 
-static IDENTITY_2Q: Matrix4<Complex64> = Matrix4::new(
-    // Row 1
-    Complex64::ONE,
-    Complex64::ZERO,
-    Complex64::ZERO,
-    Complex64::ZERO,
-    // Row 2
-    Complex64::ZERO,
-    Complex64::ONE,
-    Complex64::ZERO,
-    Complex64::ZERO,
-    // Row 3
-    Complex64::ZERO,
-    Complex64::ZERO,
-    Complex64::ONE,
-    Complex64::ZERO,
-    // Row 4
-    Complex64::ZERO,
-    Complex64::ZERO,
-    Complex64::ZERO,
-    Complex64::ONE,
-);
-
 #[allow(clippy::large_enum_variant)]
 #[derive(Clone, Debug)]
 pub enum DecomposerType {
@@ -220,7 +198,7 @@ impl PhysQargsMap {
 
 #[allow(clippy::too_many_arguments)]
 #[pyfunction]
-#[pyo3(name = "consolidate_blocks", signature = (dag, decomposer, basis_gate_name, force_consolidate, target=None, basis_gates=None, blocks=None, runs=None, qubit_map=None))]
+#[pyo3(name = "consolidate_blocks", signature = (dag, decomposer, basis_gate_name, force_consolidate, target=None, basis_gates=None, blocks=None, runs=None, qubit_map=None, approximation_degree=None))]
 fn py_run_consolidate_blocks(
     dag: &mut DAGCircuit,
     decomposer: DecomposerType,
@@ -231,7 +209,14 @@ fn py_run_consolidate_blocks(
     blocks: Option<Vec<Vec<usize>>>,
     runs: Option<Vec<Vec<usize>>>,
     qubit_map: Option<Vec<PhysicalQubit>>,
+    approximation_degree: Option<f64>,
 ) -> PyResult<()> {
+    let tol = if let Some(degree) = approximation_degree {
+        MINIMUM_TOL.max(1. - degree)
+    } else {
+        MINIMUM_TOL
+    };
+
     // The node indices that enter from `blocks` and `runs` come from Python space, and we can't
     // trust that they come from a correct analysis (or the block/run collection might have been
     // invalidated). Rather than panicking, we should raise Python-space exceptions. We don't have
@@ -424,10 +409,15 @@ fn py_run_consolidate_blocks(
                     || (basis_gates.is_some() && outside_basis)
                     || (target.is_some() && outside_basis)
                 {
-                    if approx::abs_diff_eq!(IDENTITY_2Q, matrix) {
+                    let trace = matrix.trace();
+                    let dim = 4.0;
+                    if let Some(phase_update) =
+                        average_gate_fidelity_below_tol(trace / dim, dim, tol)
+                    {
                         for node in block {
                             dag.remove_op_node(node);
                         }
+                        dag.add_global_phase(&Param::Float(phase_update))?;
                     } else {
                         let unitary_gate = UnitaryGate {
                             array: ArrayType::TwoQ(matrix),
@@ -509,10 +499,13 @@ fn py_run_consolidate_blocks(
             if already_in_block {
                 continue;
             }
-            if approx::abs_diff_eq!(aview2(&ONE_QUBIT_IDENTITY), aview2(&matrix)) {
+            let trace = matrix[0][0] + matrix[1][1];
+            let dim = 2.0;
+            if let Some(phase_update) = average_gate_fidelity_below_tol(trace / dim, dim, tol) {
                 for node in run {
                     dag.remove_op_node(node);
                 }
+                dag.add_global_phase(&Param::Float(phase_update))?;
             } else {
                 let array: Matrix2<Complex64> =
                     Matrix2::from_row_iterator(matrix.into_iter().flat_map(|x| x.into_iter()));
@@ -558,8 +551,8 @@ pub fn run_consolidate_blocks(
     approximation_degree: Option<f64>,
     target: Option<&Target>,
 ) -> PyResult<()> {
-    let approximation_degree = approximation_degree.unwrap_or(1.0);
-    let (decomposer, basis_gate) = get_decomposer_and_basis_gate(target, approximation_degree);
+    let (decomposer, basis_gate) =
+        get_decomposer_and_basis_gate(target, approximation_degree.unwrap_or(1.0));
     py_run_consolidate_blocks(
         dag,
         decomposer,
@@ -571,6 +564,7 @@ pub fn run_consolidate_blocks(
         None,
         // TODO: this doesn't handle the possibility of control-flow operations yet.
         None,
+        approximation_degree,
     )
 }
 

crates/transpiler/src/passes/remove_identity_equiv.rs

@@ -34,7 +34,7 @@ use qiskit_util::getenv_use_multiple_threads;
 // if the performance of this pass changes over time.
 const PARALLEL_THRESHOLD: usize = 50_000;
 
-const MINIMUM_TOL: f64 = 1e-12;
+pub const MINIMUM_TOL: f64 = 1e-12;
 
 /// Fidelity-based computation to check whether an operation `G` is equivalent
 /// to identity up to a global phase.

qiskit/transpiler/passes/optimization/consolidate_blocks.py

@@ -100,6 +100,7 @@ def __init__(
         super().__init__()
         self.basis_gates = None
         self.basis_gate_name = None
+        self.approximation_degree = approximation_degree
         # Bypass target if it doesn't contain any basis gates (i.e. it's a _FakeTarget), as this
         # not part of the official target model.
         self.target = target if target is not None and len(target.operation_names) > 0 else None
@@ -158,6 +159,7 @@ def run(self, dag):
             blocks=blocks,
             runs=runs,
             qubit_map=qubit_map,
+            approximation_degree=self.approximation_degree,
         )
         dag = self._handle_control_flow_ops(dag, qubit_map)
 

releasenotes/notes/fix-consolidate-block-remove-identity-c3e90f3c66b8dad2.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    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
+    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.

test/python/transpiler/test_consolidate_blocks.py

@@ -415,6 +415,18 @@ def test_identity_unitary_is_removed(self):
         pm = PassManager([Collect2qBlocks(), ConsolidateBlocks()])
         self.assertEqual(QuantumCircuit(5), pm.run(qc))
 
+    def test_identity_unitary_is_removed_up_to_phase(self):
+        """Test that a 2q identity (up to phase) unitary is removed."""
+        qc = QuantumCircuit(5)
+        qc.h(0)
+        qc.cx(0, 1)
+        qc.rz(2 * np.pi, 1)
+        qc.cx(0, 1)
+        qc.h(0)
+
+        pm = PassManager([Collect2qBlocks(), ConsolidateBlocks()])
+        self.assertEqual(QuantumCircuit(5, global_phase=np.pi), pm.run(qc))
+
     def test_identity_1q_unitary_is_removed(self):
         """Test that a 1q identity unitary is removed without a basis."""
         qc = QuantumCircuit(5)
@@ -425,6 +437,15 @@ def test_identity_1q_unitary_is_removed(self):
         pm = PassManager([Collect2qBlocks(), Collect1qRuns(), ConsolidateBlocks()])
         self.assertEqual(QuantumCircuit(5), pm.run(qc))
 
+    def test_identity_1q_unitary_is_removed_up_to_phase(self):
+        """Test that a 1q identity unitary is removed without a basis."""
+        qc = QuantumCircuit(5)
+        qc.h(0)
+        qc.rz(2 * np.pi, 0)
+        qc.h(0)
+        pm = PassManager([Collect2qBlocks(), Collect1qRuns(), ConsolidateBlocks()])
+        self.assertEqual(QuantumCircuit(5, global_phase=np.pi), pm.run(qc))
+
     def test_descent_into_control_flow(self):
         """Test consolidation in blocks when control flow op is the same as at top level."""