Skip to content

QPY 2.4rc2 fixes #15900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gadial wants to merge 9 commits into from
Closed

QPY 2.4rc2 fixes #15900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c5ce8c9
Empty commit to trigger tests
gadial Mar 30, 2026
fc566e2
Add a failing backwards compatibility test
gadial Mar 30, 2026
17011cd
Partial fix for the new test case, and better error printing
gadial Mar 30, 2026
238e5c2
Bufgix in how multiple subs for the same replay op are performed
gadial Mar 30, 2026
6bbdd14
Test + fix to the empty replay bug
gadial Mar 30, 2026
d30189e
Remove symbols that were substituted away
gadial Mar 30, 2026
a811a4e
Apply suggestions from code review
gadial Mar 31, 2026
4ca2dae
Revert one change
gadial Mar 31, 2026
775b496
Fixes according to PR review
gadial Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 87 additions & 21 deletions crates/circuit/src/parameter/parameter_expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ pub enum ParameterError {
NotASymbol,
#[error("Derivative not supported on expression: {0}")]
DerivativeNotSupported(String),
#[error("QPY replay parsing error: {0}")]
QpyReplayParsingError(String),
}

impl From<ParameterError> for PyErr {
Expand Down Expand Up @@ -260,7 +262,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> {
Comment on lines 262 to 266
Copy link
Copy Markdown
Member

@jakelishman jakelishman Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pull this function into the QPY crate as part of this patch, so we can apply the "no panicking" lint rule to it (and fix the subsequent failures)? I'm still very worried about the amount of panics here.

Copy link
Copy Markdown
Contributor Author

@gadial gadial Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how - moving it to the QPY crate creates circular dependency since ParameterExpression::__setstate__ requires from_qpy. However, I tried removing the panics from the function nevertheless in 775b496.

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();
Expand All @@ -278,24 +285,50 @@ impl ParameterExpression {

// if we need two operands, pop rhs from the stack
let rhs = if BINARY_OPS.contains(op) {
Some(stack.pop().expect("Pop from empty stack"))
Some(stack.pop().ok_or(ParameterError::QpyReplayParsingError(
"Tried to pop RHS value from empty stack".to_string(),
))?)
} else {
None
};

// pop lhs from the stack, this we always need
let lhs = stack.pop().expect("Pop from empty stack");
let lhs = stack.pop().ok_or(ParameterError::QpyReplayParsingError(
"Tried to pop LHS value from empty stack".to_string(),
))?;

// apply the operation and put the result onto the stack for the next replay
let result: ParameterExpression = match op {
OpCode::ADD => lhs.add(&rhs.unwrap())?,
OpCode::MUL => lhs.mul(&rhs.unwrap())?,
OpCode::SUB => lhs.sub(&rhs.unwrap())?,
OpCode::RSUB => rhs.unwrap().sub(&lhs)?,
OpCode::POW => lhs.pow(&rhs.unwrap())?,
OpCode::RPOW => rhs.unwrap().pow(&lhs)?,
OpCode::DIV => lhs.div(&rhs.unwrap())?,
OpCode::RDIV => rhs.unwrap().div(&lhs)?,
OpCode::ADD => lhs.add(&rhs.ok_or(ParameterError::QpyReplayParsingError(
"Missing RHS value".to_string(),
))?)?,
OpCode::MUL => lhs.mul(&rhs.ok_or(ParameterError::QpyReplayParsingError(
"Missing RHS value".to_string(),
))?)?,
OpCode::SUB => lhs.sub(&rhs.ok_or(ParameterError::QpyReplayParsingError(
"Missing RHS value".to_string(),
))?)?,
OpCode::RSUB => rhs
.ok_or(ParameterError::QpyReplayParsingError(
"Missing RHS value".to_string(),
))?
.sub(&lhs)?,
OpCode::POW => lhs.pow(&rhs.ok_or(ParameterError::QpyReplayParsingError(
"Missing RHS value".to_string(),
))?)?,
OpCode::RPOW => rhs
.ok_or(ParameterError::QpyReplayParsingError(
"Missing RHS value".to_string(),
))?
.pow(&lhs)?,
OpCode::DIV => lhs.div(&rhs.ok_or(ParameterError::QpyReplayParsingError(
"Missing RHS value".to_string(),
))?)?,
OpCode::RDIV => rhs
.ok_or(ParameterError::QpyReplayParsingError(
"Missing RHS value".to_string(),
))?
.div(&lhs)?,
OpCode::ABS => lhs.abs(),
OpCode::SIN => lhs.sin(),
OpCode::ASIN => lhs.asin(),
Expand All @@ -308,25 +341,41 @@ impl ParameterExpression {
OpCode::EXP => lhs.exp(),
OpCode::SIGN => lhs.sign(),
OpCode::GRAD | OpCode::SUBSTITUTE => {
panic!("GRAD and SUBSTITUTE are not supported.")
return Err(ParameterError::QpyReplayParsingError(
"GRAD and SUBSTITUTE are not supported.".to_string(),
));
}
};
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."))
.ok_or(ParameterError::QpyReplayParsingError("Invalid QPY replay encountered during deserialization: empty OPReplay at the end of parsing.".to_string()))?;

// need to account
result.extend_symbols(symbols);
Ok(result)
}

pub fn iter_symbols(&self) -> impl Iterator<Item = &Symbol> + '_ {
Expand Down Expand Up @@ -695,6 +744,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);
Copy link
Copy Markdown
Contributor

@raynelfss raynelfss Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to replace the name map entries that already exist? If so, this is not doing that. It will only insert the ones that are not present. If you wanted to replace the ones that exist as well you should add a call to Entry::and_modify(|_| foo()) before the or_insert call.

Copy link
Copy Markdown
Contributor Author

@gadial gadial Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't need to replace anything. The goal here is to add symbols which are not present in the reply but are present in the ParameterExpression (this happens in the Rust based implementation when we have expressions like x*0+2). In theory, all the symbols that we encounter in the replay should already be present here so we can replace, but we don't need it.

Copy link
Copy Markdown
Member

@jakelishman jakelishman Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried that we're layering hack on top of hack here. The blast radius of ParameterExpression::from_qpy not being defined in terms of native substitution is getting huge here.

I get that the trouble is coming in because OPReplay was defined in such a way that it can't represent the subs operation, but the cleanest solution (which would avoid all of these extra rewrites of the replay stream) would be to make the OpReplay struct able to represent them properly, and then just call that.

Copy link
Copy Markdown
Contributor Author

@gadial gadial Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this part of the code is not related to the subs operation; we must add the symbols that were left out of the QPY replay and were not present in the QPY file except in the symbol table. The subs operations merely tell us which of them we don't need to keep in the final count.

}
}
}

/// A parameter expression.
Expand Down Expand Up @@ -1394,7 +1454,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 {
Expand Down Expand Up @@ -1871,14 +1931,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 => {
Expand Down
63 changes: 58 additions & 5 deletions crates/qpy/src/params.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -574,7 +623,11 @@ pub(crate) fn pack_param_expression(
) -> 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() {
// 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
if let Some(symbol) = exp.try_to_symbol().ok().filter(|_| exp.num_symbols() == 1) {
match symbol.vector {
None => pack_generic_value(&GenericValue::ParameterExpressionSymbol(symbol), qpy_data),
Some(_) => pack_generic_value(
Expand Down
17 changes: 17 additions & 0 deletions test/python/circuit/test_circuit_load_from_qpy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."""

Expand Down
Loading
⚡