QPY 2.4rc2 fixes

crates/circuit/src/parameter/parameter_expression.rs

@@ -137,6 +137,10 @@ impl ParameterExpression {
         qpy_replay(self, &self.name_map, &mut replay);
         replay
     }
+    #[inline]
+    pub fn num_of_symbols(&self) -> usize {
+        self.name_map.len()
+    }
 }
 // This needs to be implemented manually, because PyO3 does not provide built-in
 // conversions for the subclasses of ParameterExpression in Python. Specifically
@@ -260,7 +264,12 @@ impl ParameterExpression {
     pub fn from_qpy(
         replay: &[OPReplay],
         subs_operations: Option<Vec<(usize, HashMap<Symbol, ParameterExpression>)>>,
+        additional_symbols: Option<&HashSet<Symbol>>,
     ) -> Result<Self, ParameterError> {
+        let mut symbols = match additional_symbols {
+            None => HashSet::new(),
+            Some(symbol_map) => symbol_map.clone(),
+        };
         // the stack contains the latest lhs and rhs values
         let mut stack: Vec<ParameterExpression> = Vec::new();
         let subs_operations = subs_operations.unwrap_or_default();
@@ -313,20 +322,34 @@ impl ParameterExpression {
             };
             stack.push(result);
             //now check whether any substitutions need to be applied at this stage
+            let mut sub_operations_to_perform = Vec::new();
+            // since we go over the operations from last to first, we need to collect all the subs
+            // for this stage and go over them in reverse order (from first to last, for this stage)
             while current_sub_operation > 0 && subs_operations[current_sub_operation - 1].0 == i + 1
             {
+                sub_operations_to_perform
+                    .push(subs_operations[current_sub_operation - 1].1.clone());
+                current_sub_operation -= 1;
+            }
+            for sub_operation in sub_operations_to_perform.iter().rev() {
                 if let Some(exp) = stack.pop() {
-                    let sub_exp = exp.subs(&subs_operations[current_sub_operation - 1].1, false)?;
+                    let sub_exp = exp.subs(sub_operation, true)?;
                     stack.push(sub_exp);
+                    for key in sub_operation.keys() {
+                        symbols.remove(key); // remove the symbols that were substituted away
+                    }
                 }
-                current_sub_operation -= 1;
             }
         }
 
         // once we're done, just return the last element in the stack
-        Ok(stack
+        let mut result = stack
             .pop()
-            .expect("Invalid QPY replay encountered during deserialization: empty OPReplay."))
+            .expect("Invalid QPY replay encountered during deserialization: empty OPReplay.");
+
+        // need to account
+        result.extend_symbols(symbols);
+        Ok(result)
     }
 
     pub fn iter_symbols(&self) -> impl Iterator<Item = &Symbol> + '_ {
@@ -695,6 +718,17 @@ impl ParameterExpression {
         }
         Ok(merged)
     }
+
+    /// Extend the symbol table with additional symbols
+    pub fn extend_symbols<I>(&mut self, symbols: I)
+    where
+        I: IntoIterator<Item = Symbol>,
+    {
+        for symbol in symbols {
+            let name = symbol.repr(false);
+            self.name_map.entry(name).or_insert(symbol);
+        }
+    }
 }
 
 /// A parameter expression.
@@ -1394,7 +1428,7 @@ impl PyParameterExpression {
     fn __setstate__(&mut self, state: (Vec<OPReplay>, Option<ParameterValueType>)) -> PyResult<()> {
         // if there a replay, load from the replay
         if !state.0.is_empty() {
-            let from_qpy = ParameterExpression::from_qpy(&state.0, None)?;
+            let from_qpy = ParameterExpression::from_qpy(&state.0, None, None)?;
             self.inner = from_qpy;
         // otherwise, load from the ParameterValueType
         } else if let Some(value) = state.1 {
@@ -1871,14 +1905,20 @@ pub enum ParameterValueType {
     VectorElement(PyParameterVectorElement),
 }
 
+impl From<Value> for ParameterValueType {
+    fn from(value: Value) -> Self {
+        match value {
+            Value::Int(i) => ParameterValueType::Int(i),
+            Value::Real(r) => ParameterValueType::Float(r),
+            Value::Complex(c) => ParameterValueType::Complex(c),
+        }
+    }
+}
+
 impl ParameterValueType {
     fn extract_from_expr(expr: &SymbolExpr) -> Option<ParameterValueType> {
         if let Some(value) = expr.eval(true) {
-            match value {
-                Value::Int(i) => Some(ParameterValueType::Int(i)),
-                Value::Real(r) => Some(ParameterValueType::Float(r)),
-                Value::Complex(c) => Some(ParameterValueType::Complex(c)),
-            }
+            Some(value.into())
         } else if let SymbolExpr::Symbol(symbol) = expr {
             match symbol.index {
                 None => {

crates/qpy/src/params.rs

@@ -10,11 +10,13 @@
 // copyright notice, and modified files need to carry a notice indicating
 // that they have been altered from the originals.
 use binrw::Endian;
+use hashbrown::HashSet;
 use pyo3::prelude::*;
 use qiskit_circuit::imports;
 use qiskit_circuit::operations::Param;
 use qiskit_circuit::parameter::parameter_expression::{
     OPReplay, OpCode, ParameterExpression, ParameterValueType, PyParameter,
+    PyParameterVectorElement,
 };
 use qiskit_circuit::parameter::symbol_expr::Symbol;
 use std::sync::Arc;
@@ -224,10 +226,53 @@ fn pack_symbol_table_element(
     }
 }
 
+// In case the parameter expression reduces to a constant value/symbol
+// we still want to pack it as an expression to save the symbol table data,
+// even though the symbols are not used. So we turn the constant to an equivalent expression.
+fn pack_parameter_expression_with_empty_replay(
+    exp: &ParameterExpression,
+) -> Result<Vec<formats::ParameterExpressionElementPack>, QpyError> {
+    // Try constant value first
+    if let Ok(value) = exp.try_to_value(false) {
+        let synthetic_op = OPReplay {
+            op: OpCode::ADD,
+            lhs: Some(value.into()),
+            rhs: Some(ParameterValueType::Int(0)),
+        };
+        return pack_parameter_expression_element(&synthetic_op);
+    }
+
+    // Check if it's a bare parameter or parameter vector element
+    if let Ok(symbol) = exp.try_to_symbol_ref() {
+        let param_value = match symbol.index {
+            None => ParameterValueType::Parameter(PyParameter {
+                symbol: symbol.clone(),
+            }),
+            Some(_) => ParameterValueType::VectorElement(PyParameterVectorElement {
+                symbol: symbol.clone(),
+            }),
+        };
+        let synthetic_op = OPReplay {
+            op: OpCode::ADD,
+            lhs: Some(param_value),
+            rhs: Some(ParameterValueType::Int(0)),
+        };
+        return pack_parameter_expression_element(&synthetic_op);
+    }
+    Err(QpyError::InvalidParameter(format!(
+        "Cannot encode parameter expression {:?}",
+        exp
+    )))
+}
+
 fn pack_parameter_expression_elements(
     exp: &ParameterExpression,
 ) -> Result<Vec<formats::ParameterExpressionElementPack>, QpyError> {
-    let mut result = Vec::new();
+    let replay = exp.qpy_replay();
+    if replay.is_empty() {
+        return pack_parameter_expression_with_empty_replay(exp);
+    }
+    let mut result: Vec<formats::ParameterExpressionElementPack> = Vec::new();
     for replay_obj in exp.qpy_replay().iter() {
         let packed_parameter = pack_parameter_expression_element(replay_obj)?;
         result.extend(packed_parameter);
@@ -453,9 +498,13 @@ pub(crate) fn unpack_parameter_expression(
             replay.push(OPReplay { op, lhs, rhs });
         };
     }
-    ParameterExpression::from_qpy(&replay, Some(sub_operations)).map_err(|_| {
-        QpyError::ConversionError("Failure while loading parameter expression".to_string())
-    })
+    let additional_symbols = HashSet::from_iter(param_uuid_map.values().filter_map(|v| match v {
+        GenericValue::ParameterExpressionSymbol(s) => Some(s.clone()),
+        _ => None,
+    }));
+    ParameterExpression::from_qpy(&replay, Some(sub_operations), Some(&additional_symbols)).map_err(
+        |_| QpyError::ConversionError("Failure while loading parameter expression".to_string()),
+    )
 }
 
 pub(crate) fn pack_symbol(symbol: &Symbol) -> formats::ParameterSymbolPack {
@@ -568,13 +617,29 @@ pub(crate) fn unpack_parameter_vector(
     })
 }
 
+// exp should be a symbol (for parameter/parameter vector element)
+// but more than that: it should not contain other symbols in its symbol table
+// since if, e.g. exp was `0*x+y` and got simplified to `y` we still need
+// to store `x`, so we must treat exp as an expression
+fn expression_as_single_symbol(exp: &ParameterExpression) -> Option<Symbol> {
+    if let Ok(symbol) = exp.try_to_symbol() {
+        if exp.num_of_symbols() == 1 {
+            Some(symbol)
+        } else {
+            None
+        }
+    } else {
+        None
+    }
+}
+
 pub(crate) fn pack_param_expression(
     exp: &ParameterExpression,
     qpy_data: &QPYWriteData,
 ) -> Result<formats::GenericDataPack, QpyError> {
     // if the parameter expression is a single symbol, we should treat it like a parameter
     // or a parameter vector, depending on whether the `vector` field exists
-    if let Ok(symbol) = exp.try_to_symbol() {
+    if let Some(symbol) = expression_as_single_symbol(exp) {
         match symbol.vector {
             None => pack_generic_value(&GenericValue::ParameterExpressionSymbol(symbol), qpy_data),
             Some(_) => pack_generic_value(

test/python/circuit/test_circuit_load_from_qpy.py

@@ -366,6 +366,23 @@ def test_parameter_expression(self):
         self.assertEqual(qc, new_circuit)
         self.assertDeprecatedBitProperties(qc, new_circuit)
 
+    def test_degenerate_parameter_expression(self):
+        """Test a circuit with a parameter expression that simplifies to 0."""
+        x = Parameter("x")
+        y_vec = ParameterVector("y", 2)
+        z = Parameter("z")
+        cases = [0 * x, 0 * x + 2, 0 * x + z, x - x, 0 * y_vec[0], 0 * (x + y_vec[1])]
+        for case in cases:
+            qc = QuantumCircuit(1)
+            qc.rz(case, 0)
+            qpy_file = io.BytesIO()
+            dump(qc, qpy_file)
+            qpy_file.seek(0)
+            new_circuit = load(qpy_file)[0]
+            self.assertEqual(qc, new_circuit)
+            # should still have the same parameters even if they are not used
+            self.assertEqual(qc.parameters, new_circuit.parameters)
+
     def test_string_parameter(self):
         """Test a PauliGate instruction that has string parameters."""
 

test/qpy_compat/test_qpy.py

@@ -861,7 +861,15 @@ def generate_replay_with_expression_substitutions():
     qc = QuantumCircuit(1)
     qc.rz(a2, 0)
 
-    return [qc]
+    qc2 = QuantumCircuit(1)
+    theta = Parameter("θ")
+    rz = Parameter("rz")
+    exp = theta + np.pi
+    exp = exp.subs({theta: rz})
+    exp = exp.subs({rz: theta})
+    qc2.rz(exp, 0)
+
+    return [qc, qc2]
 
 
 def generate_v14_expr():
@@ -1023,8 +1031,9 @@ def assert_equal(
         qpy_parameter_names = [x.name for x in qpy.parameters]
         if reference_parameter_names != qpy_parameter_names:
             msg = (
+                f"QPY ERROR: "
                 f"Circuit {count} parameter mismatch:"
-                f" {reference_parameter_names} != {qpy_parameter_names}"
+                f" {reference_parameter_names} != {qpy_parameter_names}\n"
             )
             sys.stderr.write(msg)
             sys.exit(4)
@@ -1034,6 +1043,7 @@ def assert_equal(
     if equivalent:
         if not Operator.from_circuit(reference).equiv(Operator.from_circuit(qpy)):
             msg = (
+                f"QPY ERROR: "
                 f"For {context}:\n"
                 f"Reference Circuit {count}:\n{reference}\nis not equivalent to "
                 f"qpy loaded circuit {count}:\n{qpy}\n"
@@ -1042,6 +1052,7 @@ def assert_equal(
             sys.exit(1)
     elif reference != qpy:
         msg = (
+            f"QPY ERROR: "
             f"Reference Circuit {count}:\n{reference}\nis not equivalent to "
             f"qpy loaded circuit {count}:\n{qpy}\n"
         )
@@ -1055,6 +1066,7 @@ def assert_equal(
         ):
             if ref_bit._register is not None and ref_bit != qpy_bit:
                 msg = (
+                    f"QPY ERROR: "
                     f"For {context}:\n"
                     f"Reference Circuit {count}:\n"
                     "deprecated bit-level register information mismatch\n"
@@ -1069,19 +1081,17 @@ def assert_equal(
         and isinstance(reference, QuantumCircuit)
         and reference.layout != qpy.layout
     ):
-        msg = (
-            f"For {context}:\nCircuit {count} layout mismatch {reference.layout} != {qpy.layout}\n"
-        )
+        msg = f"QPY ERROR:\nFor {context}:\nCircuit {count} layout mismatch {reference.layout} != {qpy.layout}\n"
         sys.stderr.write(msg)
         sys.exit(4)
 
     # Don't compare name on bound circuits
     if bind is None and reference.name != qpy.name:
-        msg = f"For {context}:\nCircuit {count} name mismatch {reference.name} != {qpy.name}\n{reference}\n{qpy}"
+        msg = f"QPY ERROR:\nFor {context}:\nCircuit {count} name mismatch {reference.name} != {qpy.name}\n{reference}\n{qpy}"
         sys.stderr.write(msg)
         sys.exit(2)
     if reference.metadata != qpy.metadata:
-        msg = f"For {context}:\nCircuit {count} metadata mismatch: {reference.metadata} != {qpy.metadata}"
+        msg = f"QPY ERROR:\nFor {context}:\nCircuit {count} metadata mismatch: {reference.metadata} != {qpy.metadata}"
         sys.stderr.write(msg)
         sys.exit(3)
 
@@ -1109,8 +1119,13 @@ def load_qpy(qpy_files, version_parts):
             # See https://github.com/Qiskit/qiskit/pull/13814
             continue
         print(f"Loading qpy file: {path}")  # noqa: T201
-        with open(path, "rb") as fd:
-            qpy_circuits = load(fd)
+        try:
+            with open(path, "rb") as fd:
+                qpy_circuits = load(fd)
+        except Exception as ex:
+            msg = f"QPY Error: Failed to load {path} with the exception: {ex}\n"
+            sys.stderr.write(msg)
+            sys.exit(1)
         equivalent = path in {"open_controlled_gates.qpy", "controlled_gates.qpy"}
         for i, circuit in enumerate(circuits):
             bind = None
@@ -1153,7 +1168,7 @@ def load_qpy(qpy_files, version_parts):
                 with open(path, "rb") as fd:
                     load(fd)
             except Exception:
-                msg = "Loading circuit with pulse gates should not raise"
+                msg = "QPY ERROR:\nLoading circuit with pulse gates should not raise\n"
                 sys.stderr.write(msg)
                 sys.exit(1)
         else:
@@ -1164,7 +1179,7 @@ def load_qpy(qpy_files, version_parts):
             except QpyError:
                 continue
 
-            msg = f"Loading payload {path} didn't raise QpyError"
+            msg = f"QPY ERROR:\nLoading payload {path} didn't raise QpyError\n"
             sys.stderr.write(msg)
             sys.exit(1)