From 21fdcef68b59dc361d99da8fac1d54ba4b9dd09d Mon Sep 17 00:00:00 2001 From: Jake Lishman Date: Thu, 21 May 2026 12:16:14 +0100 Subject: [PATCH] Move `ParameterVector` to Rust `ParameterVector` was previously a pure-Python concept, even though the Rust-space `Symbol` necessarily had to track an internal backreference to a vector Python object in order to convert itself to Python later. This hidden dependence on the Python interpreter was the root cause of a panicking bug in Qiskit 2.4[^1]. All `ParameterVectorElement`s in Python space have names and UUIDs that are pure functions of the underlying vector and the element's index. Now that this information is available in Rust space, it is far more efficient to simply not store all this derived information, but calculate it on the fly, as needed. This motivated the change in `Symbol` to an `enum`: the logic was _already_ handling the two cases of "standalone" and "element", but this formally separates them to avoid storing unnecessary data. Doing this alone is already a performance benefit: construction of a `list(ParameterVector("a", 10_000))` (to ensure all the same Python-space objects are created) went from 7.7ms to 1.4ms on my machine (~5x speedup). This work here is far more aggressive at threading through shared `Arc` references of both `Symbol` and `SymbolVector`. The `Arc` is necessary for correctness in Python space, not just performance: we must have `all(el.vector == els[0].vector for el in els)`, and we don't want to have to cache more Python objects within `Symbol` to achieve that. Unfortunately, `ParameterVector` never implemented true equality (only the default referential equality that Python uses to make mutable-state objects safely hashable by default), so we certainly need some other stable reference that can be shared. The previous Python-space constructions of `PyParameter` and `PyParameterVectorElement` had not been done entirely correctly with respect to subclassing; the same (cloned) `Symbol` was present in multiple places, which was two more allocations than necessary (most places in Rust use `Arc`, which can be near-freely cloned). This corrects it so that `PyParameterVectorElement` is now just a simple marker type, and the majority of the Rust logic needn't concern itself with it at all. The `AtomicUsize` used for the length of `SymbolVector` is to support the unfortunate `ParameterVector.resize` operation from Python space. The atomic operation makes the whole logic safe from data races, but does nothing to solve the potential race condition of two places querying a vector its length concurrently and getting different answers, or the problem that shortening the vector can invalidate already-held references to `ParameterVectorElement`s or `Symbol`s (a problem that was pre-existing in Python). The `resize` in-place method is fairly fundamentally unsound for these reasons, and we should consider removing it; it's only used in the deprecated `NLocal` subclass of `BlueprintCircuit`, and that use can probably be replaced by a simple lazier construction of the vector, or a complete object replacement rather than mutation. [^1]: 16c808857f: Fix panic in `RemoveIdentityEquivalent` with `ParameterVector` global phase (gh-16054) --- crates/cext/src/param.rs | 2 +- .../transpiler/passes/unitary_synthesis.rs | 18 +- crates/cext/src/transpiler/target.rs | 2 +- crates/circuit/src/circuit_drawer.rs | 15 +- crates/circuit/src/lib.rs | 1 + crates/circuit/src/operations.rs | 6 +- .../src/parameter/parameter_expression.rs | 598 ++++++++++-------- crates/circuit/src/parameter/symbol_expr.rs | 208 +++--- crates/circuit/src/parameter/symbol_parser.rs | 30 +- crates/circuit/src/parameter_table.rs | 13 +- crates/qasm3/src/exporter.rs | 5 +- crates/qpy/src/circuit_reader.rs | 20 +- crates/qpy/src/circuit_writer.rs | 2 +- crates/qpy/src/params.rs | 198 +++--- crates/qpy/src/py_methods.rs | 26 +- crates/qpy/src/value.rs | 12 +- .../basis_translator/compose_transforms.rs | 15 +- .../src/standard_equivalence_library.rs | 25 +- crates/transpiler/src/target/mod.rs | 36 +- crates/transpiler/src/transpiler.rs | 18 +- qiskit/circuit/parametervector.py | 127 +--- qiskit/qpy/binary_io/value.py | 2 +- ...ust-parameter-vector-3fbac07411c36650.yaml | 4 + test/python/circuit/test_parameters.py | 7 + 24 files changed, 656 insertions(+), 734 deletions(-) create mode 100644 releasenotes/notes/rust-parameter-vector-3fbac07411c36650.yaml diff --git a/crates/cext/src/param.rs b/crates/cext/src/param.rs index 24f649be3a0c..f75ea6c6b4ed 100644 --- a/crates/cext/src/param.rs +++ b/crates/cext/src/param.rs @@ -47,7 +47,7 @@ pub unsafe extern "C" fn qk_param_new_symbol(name: *const c_char) -> *mut Param // Per documentation, the name cannot be empty. panic!("Invalid empty name."); } else { - let symbol = Symbol::new(name, None, None); + let symbol = Symbol::standalone(name.to_owned(), None); let expr = ParameterExpression::from_symbol(symbol); let param = Param::ParameterExpression(Arc::new(expr)); Box::into_raw(Box::new(param)) diff --git a/crates/cext/src/transpiler/passes/unitary_synthesis.rs b/crates/cext/src/transpiler/passes/unitary_synthesis.rs index f714c7a7117a..72fd5d5ee5c2 100644 --- a/crates/cext/src/transpiler/passes/unitary_synthesis.rs +++ b/crates/cext/src/transpiler/passes/unitary_synthesis.rs @@ -181,15 +181,15 @@ mod tests { ) .unwrap(); let params = Some(Parameters::Params(smallvec![ - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "ϴ", None, None, - )))), - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "φ", None, None, - )))), - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "λ", None, None, - )))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("ϴ".to_owned(), None) + ))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("φ".to_owned(), None) + ))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("λ".to_owned(), None) + ))), ])); target .add_instruction( diff --git a/crates/cext/src/transpiler/target.rs b/crates/cext/src/transpiler/target.rs index 30bd7df8f1d5..5ea1da6cb164 100644 --- a/crates/cext/src/transpiler/target.rs +++ b/crates/cext/src/transpiler/target.rs @@ -509,7 +509,7 @@ impl TargetEntry { .map(|i| { let op_name = operation.name(); Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( - Symbol::new(format!("{op_name}_param_{i}").as_str(), None, None), + Symbol::standalone(format!("{op_name}_param_{i}"), None), ))) }) .collect(), diff --git a/crates/circuit/src/circuit_drawer.rs b/crates/circuit/src/circuit_drawer.rs index fa48f7598910..fd09e2c496a5 100644 --- a/crates/circuit/src/circuit_drawer.rs +++ b/crates/circuit/src/circuit_drawer.rs @@ -2082,7 +2082,7 @@ c2: 2/══════════ let mut circuit = basic_circuit(); circuit .set_global_phase_param(Param::ParameterExpression(Arc::new( - ParameterExpression::from_symbol(Symbol::new("ϕ", None, None)), + ParameterExpression::from_symbol(Symbol::standalone("ϕ".to_owned(), None)), ))) .unwrap(); let result = draw_circuit(&circuit, true, false, Some(80)).unwrap(); @@ -2112,7 +2112,7 @@ c2: 2/══════════ ]; let mut circuit = CircuitData::new(Some(qubits), None, Param::Float(0.0)).unwrap(); let param = Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( - Symbol::new("a", None, None), + Symbol::standalone("a".to_owned(), None), ))); circuit .push_standard_gate(StandardGate::RXX, &[param], &[Qubit(0), Qubit(1)]) @@ -2276,7 +2276,7 @@ c_3: ═════════════════════════ ]; let mut circuit = CircuitData::new(Some(qubits), None, Param::Float(0.0)).unwrap(); let param = Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( - Symbol::new("ϕ", None, None), + Symbol::standalone("ϕ".to_owned(), None), ))); circuit .push_standard_gate(StandardGate::RXX, &[param], &[Qubit(0), Qubit(1)]) @@ -2311,7 +2311,7 @@ q_1: ┤1 ├┤1 ├┤1 ├ ]; let mut circuit = CircuitData::new(Some(qubits), None, Param::Float(0.0)).unwrap(); let param = Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( - Symbol::new("🎩", None, None), + Symbol::standalone("🎩".to_owned(), None), ))); circuit .push_standard_gate(StandardGate::RY, std::slice::from_ref(¶m), &[Qubit(1)]) @@ -2360,7 +2360,7 @@ q_1: ┤ Ry(🎩) ├┤1 ├─┤ 💶🔉(🎩) ├─┤1 .push_standard_gate(StandardGate::RX, &[Param::Float(123.4567)], &[Qubit(0)]) .unwrap(); - let expr = ParameterExpression::from_symbol(Symbol::new("ϕ", None, None)) + let expr = ParameterExpression::from_symbol(Symbol::standalone("ϕ".to_owned(), None)) .mul(&ParameterExpression::from_f64(1.23456)) .unwrap(); let param = Param::ParameterExpression(Arc::new(expr)); @@ -2493,8 +2493,9 @@ q_1: ┤ Rz(1.2346e8) ├┤ Rx(0.12346) ├┤ Rx(1.2346e-5) ├┤ Rx(2π/3) ) .unwrap(); - let theta = Arc::new(ParameterExpression::from_symbol(Symbol::new( - "θ", None, None, + let theta = Arc::new(ParameterExpression::from_symbol(Symbol::standalone( + "θ".to_owned(), + None, ))); circuit diff --git a/crates/circuit/src/lib.rs b/crates/circuit/src/lib.rs index 8542f25a35f4..2e553943a8db 100644 --- a/crates/circuit/src/lib.rs +++ b/crates/circuit/src/lib.rs @@ -240,6 +240,7 @@ pub fn circuit(m: &Bound) -> PyResult<()> { m.add_class::()?; m.add_class::()?; m.add_class::()?; + m.add_class::()?; m.add_class::()?; m.add_class::()?; let classical_mod = PyModule::new(m.py(), "classical")?; diff --git a/crates/circuit/src/operations.rs b/crates/circuit/src/operations.rs index 8bb42334d143..ef458c9e5b71 100644 --- a/crates/circuit/src/operations.rs +++ b/crates/circuit/src/operations.rs @@ -150,11 +150,7 @@ impl Param { .getattr(parameters_attr)? .try_iter()? .map(|elem| { - let elem = elem?; - let py_param_bound = elem.cast::()?; - let py_param = py_param_bound.borrow(); - let symbol = py_param.symbol(); - Ok(symbol.clone()) + Ok(Symbol::clone(&elem?.cast_into::()?.borrow().0)) }) .collect::>()?; Ok(Box::new(collected.into_iter())) diff --git a/crates/circuit/src/parameter/parameter_expression.rs b/crates/circuit/src/parameter/parameter_expression.rs index 7f3aeea51f2b..7fe9a731f1ea 100644 --- a/crates/circuit/src/parameter/parameter_expression.rs +++ b/crates/circuit/src/parameter/parameter_expression.rs @@ -13,10 +13,7 @@ use hashbrown::hash_map::Entry; use hashbrown::{HashMap, HashSet}; use num_complex::Complex64; -use pyo3::exceptions::{PyRuntimeError, PyTypeError, PyValueError, PyZeroDivisionError}; -use pyo3::types::{IntoPyDict, PyComplex, PyFloat, PyInt, PyNotImplemented, PySet, PyString}; -use qiskit_util::IndexSet; -use std::sync::Arc; +use std::sync::{Arc, atomic}; use thiserror::Error; use uuid::Uuid; @@ -24,14 +21,22 @@ use std::collections::hash_map::DefaultHasher; use std::fmt; use std::hash::{Hash, Hasher}; -use pyo3::IntoPyObjectExt; +use pyo3::exceptions::{ + PyDeprecationWarning, PyRuntimeError, PyTypeError, PyValueError, PyZeroDivisionError, +}; use pyo3::prelude::*; +use pyo3::sync::PyOnceLock; +use pyo3::types::{ + IntoPyDict, PyComplex, PyFloat, PyInt, PyIterator, PyList, PyNotImplemented, PySet, PyString, + PyTuple, +}; +use pyo3::{IntoPyObjectExt, intern}; use crate::circuit_data::CircuitError; -use crate::imports::{BUILTIN_HASH, SYMPIFY_PARAMETER_EXPRESSION, UUID}; -use crate::parameter::symbol_expr; -use crate::parameter::symbol_expr::SymbolExpr; +use crate::imports::{BUILTIN_HASH, SYMPIFY_PARAMETER_EXPRESSION, UUID, WARNINGS_WARN}; +use crate::parameter::symbol_expr::{self, SymbolExpr, SymbolVector}; use crate::parameter::symbol_parser::parse_expression; +use qiskit_util::IndexSet; use super::symbol_expr::{SYMEXPR_EPSILON, Symbol, Value}; @@ -182,7 +187,7 @@ impl ParameterExpression { for symbol in unused { replay.push(OPReplay { op: OpCode::MUL, - lhs: Some(ParameterValueType::from_symbol(symbol)), + lhs: Some(ParameterValueType::Parameter(PyParameter(Arc::new(symbol)))), rhs: Some(ParameterValueType::Int(0)), }); replay.push(OPReplay { @@ -240,6 +245,13 @@ impl ParameterExpression { } } + pub fn from_arc_symbol(symbol: Arc) -> Self { + Self { + name_map: [(symbol.repr(false), Symbol::clone(&*symbol))].into(), + expr: SymbolExpr::Symbol(symbol), + } + } + /// Try casting to a [Symbol]. /// /// This only succeeds if the underlying expression is, in fact, only a symbol. @@ -801,10 +813,8 @@ impl PyParameterExpression { return Err(ParameterError::InvalidValue.into()); } Ok(ParameterExpression::new(SymbolExpr::Value(Value::from(c)), HashMap::new()).into()) - } else if let Ok(element) = ob.cast::() { - Ok(ParameterExpression::from_symbol(element.borrow().symbol.clone()).into()) } else if let Ok(parameter) = ob.cast::() { - Ok(ParameterExpression::from_symbol(parameter.borrow().symbol.clone()).into()) + Ok(ParameterExpression::from_symbol(Symbol::clone(¶meter.borrow().0)).into()) } else { ob.extract::().map_err(Into::into) } @@ -818,11 +828,7 @@ impl PyParameterExpression { Value::Complex(c) => Ok(PyComplex::from_complex_bound(py, c).unbind().into_any()), } } else if let Ok(symbol) = self.inner.try_to_symbol() { - if symbol.index.is_some() { - Ok(Py::new(py, PyParameterVectorElement::from_symbol(symbol))?.into_any()) - } else { - Ok(Py::new(py, PyParameter::from_symbol(symbol))?.into_any()) - } + PyParameter(Arc::new(symbol)).into_py_any(py) } else { self.clone().into_py_any(py) } @@ -835,37 +841,24 @@ impl PyParameterExpression { /// It is subject to arbitrary change in between Qiskit versions and cannot be relied on. /// Parameter expressions should always be constructed from applying operations on /// parameters, or by loading via QPY. - /// - /// The input values are allowed to be None for pickling purposes. #[new] - #[pyo3(signature = (name_map=None, expr=None))] - pub fn py_new( - name_map: Option>, - expr: Option, - ) -> PyResult { - match (name_map, expr) { - (None, None) => Ok(Self::default()), - (Some(name_map), Some(expr)) => { - // We first parse the expression and then update the symbols with the ones - // the user provided. The replacement relies on the names to match. - // This is hacky and we likely want a more reliably conversion from a SymPy object, - // if we decide we want to continue supporting this. - let expr = parse_expression(&expr) - .map_err(|_| PyRuntimeError::new_err("Failed parsing input expression"))?; - let symbol_map: HashMap = name_map - .iter() - .map(|(string, param)| (string.clone(), param.symbol.clone())) - .collect(); - - let replaced_expr = symbol_expr::replace_symbol(&expr, &symbol_map); - - let inner = ParameterExpression::new(replaced_expr, symbol_map); - Ok(Self { inner }) - } - _ => Err(PyValueError::new_err( - "Pass either both a name_map and expr, or neither", - )), - } + #[pyo3(signature = (name_map, expr))] + pub fn py_new(name_map: HashMap, expr: String) -> PyResult { + // We first parse the expression and then update the symbols with the ones + // the user provided. The replacement relies on the names to match. + // This is hacky and we likely want a more reliably conversion from a SymPy object, + // if we decide we want to continue supporting this. + let expr = parse_expression(&expr) + .map_err(|_| PyRuntimeError::new_err("Failed parsing input expression"))?; + let symbol_map: HashMap = name_map + .iter() + .map(|(string, param)| (string.clone(), Symbol::clone(¶m.0))) + .collect(); + + let replaced_expr = symbol_expr::replace_symbol(&expr, &symbol_map); + + let inner = ParameterExpression::new(replaced_expr, symbol_map); + Ok(Self { inner }) } #[allow(non_snake_case)] @@ -929,16 +922,7 @@ impl PyParameterExpression { let py_parameters: Vec> = self .inner .iter_symbols() - .map(|symbol| { - if symbol.is_vector_element() { - Ok( - Py::new(py, PyParameterVectorElement::from_symbol(symbol.clone()))? - .into_any(), - ) - } else { - Ok(Py::new(py, PyParameter::from_symbol(symbol.clone()))?.into_any()) - } - }) + .map(|symbol| PyParameter(Arc::new(symbol.clone())).into_py_any(py)) .collect::>()?; PySet::new(py, py_parameters) } @@ -1035,18 +1019,17 @@ impl PyParameterExpression { /// Returns: /// The derivative as either a constant numeric value or a symbolic /// :class:`.ParameterExpression`. - pub fn gradient(&self, param: &Bound<'_, PyAny>) -> PyResult> { - let symbol = symbol_from_py_parameter(param)?; - let d_expr = self.inner.derivative(&symbol)?; + pub fn gradient(&self, py: Python, param: &PyParameter) -> PyResult> { + let d_expr = self.inner.derivative(¶m.0)?; // try converting to value and return as built-in numeric type match d_expr.try_to_value(false) { Ok(val) => match val { - Value::Real(r) => r.into_py_any(param.py()), - Value::Int(i) => i.into_py_any(param.py()), - Value::Complex(c) => c.into_py_any(param.py()), + Value::Real(r) => r.into_py_any(py), + Value::Int(i) => i.into_py_any(py), + Value::Complex(c) => c.into_py_any(py), }, - Err(_) => PyParameterExpression::from(d_expr).into_py_any(param.py()), + Err(_) => PyParameterExpression::from(d_expr).into_py_any(py), } } @@ -1091,7 +1074,7 @@ impl PyParameterExpression { // reduce the map to a HashMap let map = parameter_map .into_iter() - .map(|(param, expr)| Ok((param.symbol, expr.inner))) + .map(|(param, expr)| Ok((Arc::unwrap_or_clone(param.0), expr.inner))) .collect::>()?; // apply to the inner expression @@ -1132,7 +1115,7 @@ impl PyParameterExpression { .iter() .map(|(param, value)| { let value = value.extract()?; - Ok((param.symbol(), value)) + Ok((param.0.as_ref(), value)) }) .collect::>()?; @@ -1405,13 +1388,16 @@ impl PyParameterExpression { } } - fn __getstate__(&self) -> Vec { - self._qpy_replay() + #[staticmethod] + fn _reconstruct(replay: Vec) -> PyResult { + Ok(Self { + inner: ParameterExpression::from_qpy(&replay)?, + }) } - - fn __setstate__(&mut self, state: Vec) -> PyResult<()> { - self.inner = ParameterExpression::from_qpy(&state)?; - Ok(()) + fn __reduce__<'py>(&self, py: Python<'py>) -> PyResult> { + py.get_type::() + .getattr("_reconstruct") + .and_then(|meth| (meth, (self.inner.qpy_replay(),)).into_pyobject(py)) } #[getter] @@ -1459,50 +1445,35 @@ impl PyParameterExpression { module="qiskit._accelerate.circuit", extends=PyParameterExpression, name="Parameter", - from_py_object + from_py_object, + frozen, )] -#[derive(Clone, Debug, PartialEq, Eq, PartialOrd)] -pub struct PyParameter { - pub symbol: Symbol, -} - -impl Hash for PyParameter { - fn hash(&self, state: &mut H) { - self.symbol.hash(state); +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Hash)] +pub struct PyParameter(pub Arc); +impl From for PyParameter { + fn from(val: Symbol) -> Self { + Self(Arc::new(val)) } } // This needs to be implemented manually, since PyO3 does not provide this conversion // for subclasses. impl<'py> IntoPyObject<'py> for PyParameter { - type Target = PyParameter; + type Target = PyAny; type Output = Bound<'py, Self::Target>; type Error = PyErr; + /// `PyParameter`'s implementation of this is responsible for sorting out the vector-element + /// logic as well. fn into_pyobject(self, py: Python<'py>) -> Result { - let symbol = &self.symbol; - let symbol_expr = SymbolExpr::Symbol(Arc::new(symbol.clone())); - let expr = ParameterExpression::from_symbol_expr(symbol_expr); - let py_expr = PyParameterExpression::from(expr); - - Ok(Py::new(py, (self, py_expr))?.into_bound(py)) - } -} - -impl PyParameter { - /// Get a Python class initialization from a symbol. - pub fn from_symbol(symbol: Symbol) -> PyClassInitializer { - let expr = SymbolExpr::Symbol(Arc::new(symbol.clone())); - - let py_parameter = Self { symbol }; - let py_expr: PyParameterExpression = ParameterExpression::from_symbol_expr(expr).into(); - - PyClassInitializer::from(py_expr).add_subclass(py_parameter) - } - - /// Get a reference to the underlying symbol. - pub fn symbol(&self) -> &Symbol { - &self.symbol + let is_element = matches!(&*self.0, Symbol::Element { .. }); + let inner = ParameterExpression::from_symbol_expr(SymbolExpr::Symbol(self.0.clone())); + let init = PyClassInitializer::from(PyParameterExpression { inner }).add_subclass(self); + if is_element { + Bound::new(py, init.add_subclass(PyParameterVectorElement)).map(|ob| ob.into_any()) + } else { + Bound::new(py, init).map(|ob| ob.into_any()) + } } } @@ -1519,25 +1490,17 @@ impl PyParameter { /// allows them to be equal. This is useful during serialization and deserialization. #[new] #[pyo3(signature = (name, uuid=None))] - fn py_new( - py: Python<'_>, - name: String, - uuid: Option>, - ) -> PyResult> { - let uuid = uuid_from_py(py, uuid)?; - let symbol = Symbol::new(name.as_str(), uuid, None); - let expr = SymbolExpr::Symbol(Arc::new(symbol.clone())); - - let py_parameter = Self { symbol }; - let py_expr: PyParameterExpression = ParameterExpression::from_symbol_expr(expr).into(); - - Ok(PyClassInitializer::from(py_expr).add_subclass(py_parameter)) + fn py_new(name: String, uuid: Option>) -> PyResult> { + let uuid = uuid.map(uuid_from_py).transpose()?; + let symbol = Arc::new(Symbol::standalone(name, uuid)); + let inner = ParameterExpression::from_symbol_expr(SymbolExpr::Symbol(symbol.clone())); + Ok(PyClassInitializer::from(PyParameterExpression { inner }).add_subclass(Self(symbol))) } /// Returns the name of the :class:`.Parameter`. #[getter] fn name<'py>(&self, py: Python<'py>) -> Bound<'py, PyString> { - PyString::new(py, &self.symbol.repr(false)) + PyString::new(py, &self.0.repr(false)) } /// Returns the :class:`~uuid.UUID` of the :class:`Parameter`. @@ -1546,28 +1509,29 @@ impl PyParameter { /// :class:`.Parameter` constructor to produce an instance that compares /// equal to another instance. #[getter] - fn uuid(&self, py: Python<'_>) -> PyResult> { - uuid_to_py(py, self.symbol.uuid) + fn uuid<'py>(&self, py: Python<'py>) -> PyResult> { + uuid_to_py(py, self.0.uuid()) } pub fn __repr__<'py>(&self, py: Python<'py>) -> Bound<'py, PyString> { - let str = format!("Parameter({})", self.symbol.repr(false),); + let str = format!("Parameter({})", self.0.repr(false),); PyString::new(py, str.as_str()) } pub fn __getnewargs__(&self) -> (String, u128) { - (self.symbol.repr(false), self.symbol.uuid.as_u128()) - } - - pub fn __getstate__(&self) -> (String, u128) { - (self.symbol.repr(false), self.symbol.uuid.as_u128()) + (self.0.repr(false), self.0.uuid().as_u128()) } - pub fn __setstate__(&mut self, state: (String, u128)) { - let name = state.0.as_str(); - let uuid = Uuid::from_u128(state.1); - let symbol = Symbol::new(name, Some(uuid), None); - self.symbol = symbol; + // In Python space we inherit from `PyParameterExpression`, but we don't want its `__reduce__` + // to take effect. This statement is the same as the Python statement: + // + // class Paramemter: + // __reduce__ = object.__reduce__ + // + // which is a standard way of overriding with the default. + #[classattr] + fn __reduce__(py: Python) -> PyResult> { + py.get_type::().getattr("__reduce__") } fn __copy__(slf: PyRef) -> PyRef { @@ -1693,126 +1657,73 @@ impl PyParameter { module="qiskit._accelerate.circuit", extends=PyParameter, name="ParameterVectorElement", - from_py_object + skip_from_py_object )] #[derive(Clone, Debug, Eq, PartialEq, PartialOrd)] -pub struct PyParameterVectorElement { - pub symbol: Symbol, -} - -impl<'py> IntoPyObject<'py> for PyParameterVectorElement { - type Target = PyParameterVectorElement; - type Output = Bound<'py, Self::Target>; - type Error = PyErr; - - fn into_pyobject(self, py: Python<'py>) -> Result { - let symbol = &self.symbol; - let py_param = PyParameter::from_symbol(symbol.clone()); - let py_element = py_param.add_subclass(self); - - Ok(Py::new(py, py_element)?.into_bound(py)) - } -} - -impl PyParameterVectorElement { - pub fn symbol(&self) -> &Symbol { - &self.symbol - } - - pub fn from_symbol(symbol: Symbol) -> PyClassInitializer { - let py_element = Self { - symbol: symbol.clone(), - }; - let py_parameter = PyParameter::from_symbol(symbol); - - py_parameter.add_subclass(py_element) - } -} +// This carries no data in Rust space; `PyParameter`/`Symbol` already has it all. It's just a +// Python type-level marker. Nothing in Rust space should actually use this. +pub(crate) struct PyParameterVectorElement; #[pymethods] impl PyParameterVectorElement { #[new] #[pyo3(signature = (vector, index, uuid=None))] - pub fn py_new( - py: Python<'_>, - vector: Py, - index: u32, - uuid: Option>, - ) -> PyResult> { - let vector_name = vector.getattr(py, "name")?.extract::(py)?; - let uuid = uuid_from_py(py, uuid)?.unwrap_or(Uuid::new_v4()); - - let symbol = Symbol::py_new( - &vector_name, - Some(uuid.as_u128()), - Some(index), - Some(vector.clone_ref(py)), - )?; - - let py_parameter = PyParameter::from_symbol(symbol.clone()); - let py_element = Self { symbol }; - - Ok(py_parameter.add_subclass(py_element)) - } - - pub fn __getnewargs__(&self, py: Python) -> PyResult<(Py, u32, Option>)> { - let vector = self - .symbol - .vector - .clone() - .expect("vector element should have a vector"); - let index = self - .symbol - .index - .expect("vector element should have an index"); - let uuid = uuid_to_py(py, self.symbol.uuid)?; - Ok((vector, index, Some(uuid))) - } - - pub fn __repr__<'py>(&self, py: Python<'py>) -> Bound<'py, PyString> { - let str = format!("ParameterVectorElement({})", self.symbol.repr(false),); - PyString::new(py, str.as_str()) + pub fn py_new<'py>( + py: Python<'py>, + vector: &PyParameterVector, + index: usize, + uuid: Option>, + ) -> PyResult> { + if uuid.is_some() { + let msg = intern!( + py, + "constructing a `ParameterVectorElement` with an explicit UUID is no longer \ + supported; UUIDs are always derived from the base vector." + ); + WARNINGS_WARN + .get_bound(py) + .call1((msg, py.get_type::()))?; + } + let symbol = Symbol::Element { + index, + base: vector.base.clone(), + }; + PyParameter(Arc::new(symbol)).into_py_any(py) } - pub fn __getstate__(&self, py: Python) -> PyResult<(Py, u32, Option>)> { - self.__getnewargs__(py) + pub fn __getnewargs__(self_: PyRef) -> (PyParameterVector, usize) { + match &*self_.as_super().0 { + Symbol::Standalone { .. } => { + panic!("internal logic error: standalone symbols cannot become vector elements") + } + Symbol::Element { base, index } => (PyParameterVector::new(base.clone()), *index), + } } - pub fn __setstate__( - &mut self, - py: Python, - state: (Py, u32, Option>), - ) -> PyResult<()> { - let vector = state.0; - let index = state.1; - let vector_name = vector.getattr(py, "name")?.extract::(py)?; - let uuid = uuid_from_py(py, state.2)?.map(|id| id.as_u128()); - self.symbol = Symbol::py_new(&vector_name, uuid, Some(index), Some(vector))?; - Ok(()) + pub fn __repr__(self_: PyRef) -> String { + format!("ParameterVectorElement({})", self_.as_super().0.repr(false)) } /// Get the index of this element in the parent vector. #[getter] - pub fn index(&self) -> u32 { - self.symbol - .index - .expect("A vector element should have an index") + pub fn index(self_: PyRef) -> usize { + match &*self_.as_super().0 { + Symbol::Standalone { .. } => { + panic!("internal logic error: standalone symbols cannot become vector elements") + } + Symbol::Element { index, base: _ } => *index, + } } /// Get the parent vector instance. #[getter] - pub fn vector(&self) -> Py { - self.symbol - .clone() - .vector - .expect("A vector element should have a vector") - } - - /// For backward compatibility only. This should not be used and we ought to update those - /// usages! - #[getter] - pub fn _vector(&self) -> Py { - self.vector() + pub fn vector(self_: PyRef) -> PyParameterVector { + match &*self_.as_super().0 { + Symbol::Standalone { .. } => { + panic!("internal logic error: standalone symbols cannot become vector elements") + } + Symbol::Element { base, index: _ } => PyParameterVector::new(base.clone()), + } } fn __copy__(slf: PyRef) -> PyRef { @@ -1826,42 +1737,184 @@ impl PyParameterVectorElement { } } +/// A container of many related :class:`Parameter` objects. +/// +/// This class is faster to construct than constructing many :class:`.Parameter` objects +/// individually, and the individual names of the parameters will all share a common stem (the name +/// of the vector). For a vector called ``v`` with length 3, the individual elements will have +/// names ``v[0]``, ``v[1]`` and ``v[2]``. +/// +/// The elements of a vector are sorted by the name of the vector, then the numeric value of their +/// index. +/// +/// This class fulfills the :class:`collections.abc.Sequence` interface. +#[pyclass( + sequence, + module = "qiskit._accelerate.circuit", + name = "ParameterVector", + skip_from_py_object +)] +pub struct PyParameterVector { + base: Arc, + /// Cache of the name list. + name: PyOnceLock>, + /// Cache of the parameter list. + params: PyOnceLock>, +} +impl PyParameterVector { + pub fn new(base: Arc) -> Self { + Self { + base, + name: PyOnceLock::new(), + params: PyOnceLock::new(), + } + } +} +#[pymethods] +impl PyParameterVector { + #[new] + #[pyo3(signature = (name, length=0, uuid=None))] + fn py_new(name: Bound, length: usize, uuid: Option>) -> PyResult { + let base = Arc::new(SymbolVector { + name: name.to_str()?.to_owned(), + uuid: uuid + .map(uuid_from_py) + .transpose()? + .unwrap_or_else(Uuid::new_v4), + len: length.into(), + }); + // Annoyingly, PyO3 doesn't have the same `impl From for OnceLock`. + let name_cell = PyOnceLock::new(); + name_cell + .set(name.py(), name.unbind()) + .expect("the line above is the only reference, so we cannot be racing a thread"); + Ok(Self { + base, + name: name_cell, + params: PyOnceLock::new(), + }) + } + + /// The base name of the vector. + #[getter] + fn name<'py>(&self, py: Python<'py>) -> Bound<'py, PyString> { + self.name + .get_or_init(py, || PyString::new(py, &self.base.name).unbind()) + .bind(py) + .clone() + } + + /// A sequence of the contained :class:`.ParameterVectorElement` instances. + /// + /// Mutations to this object are not tracked. + #[getter] + fn params<'py>(&self, py: Python<'py>) -> PyResult> { + self.params + .get_or_try_init(py, || { + PyList::new(py, self.base.iter().map(PyParameter::from)).map(|x| x.unbind()) + }) + .map(|ob| ob.bind(py).clone()) + } + + /// Get the root UUID of this vector. + #[getter] + fn uuid<'py>(&self, py: Python<'py>) -> PyResult> { + uuid_to_py(py, self.base.uuid) + } + + // Historically, `ParameterVector` _did not_ implement `__eq__` and `__hash__`, or more + // precisely, it inherited the `object` ones, which are based on pointer identity. We can't + // have the natural definitions of `__eq__` and `__hash__` while the mutable `resize` exists, so + // instead we reinvent pointer-identity equality and hashing in a way that works with the + // on-the-fly Python-object creation from raw `Arc` instances. + fn __eq__(&self, other: &Self) -> bool { + Arc::ptr_eq(&self.base, &other.base) + } + fn __hash__(&self) -> usize { + Arc::as_ptr(&self.base).addr() + } + + /// Find the index of a :class:`.ParameterVectorElement` within the sequence. + /// + /// This is an inefficient way of accessing the :attr:`~.ParameterVectorElement.index` field on + /// the given value, which you should use instead. + fn index(&self, val: &Bound) -> PyResult { + self.params(val.py())?.index(val) + } + fn __getitem__<'py>(&self, val: &Bound<'py, PyAny>) -> PyResult> { + self.params(val.py())?.as_any().get_item(val) + } + fn __iter__<'py>(&self, py: Python<'py>) -> PyResult> { + self.params(py)?.as_any().try_iter() + } + fn __len__(&self) -> usize { + self.base.len.load(atomic::Ordering::Relaxed) + } + + fn __str__(&self) -> String { + // Yeah, I don't know why we do this either, but it's been like this ever since + // `ParameterVector` was first introduced. + format!( + "{}, [{}]", + &self.base.name, + self.base + .iter() + .map(|s| format!("'{}'", s.fullname())) + .collect::>() + .join(", ") + ) + } + fn __repr__(&self) -> String { + format!( + "ParameterVector(name='{}', length={})", + &self.base.name, + self.base.len.load(atomic::Ordering::Relaxed) + ) + } + + fn __getnewargs__<'py>(&self, py: Python<'py>) -> PyResult> { + ( + self.name(py), + self.base.len.load(atomic::Ordering::Relaxed), + uuid_to_py(py, self.base.uuid)?, + ) + .into_pyobject(py) + } + + /// Change the length of this vector. + /// + /// Any references to :class:`.ParameterVectorElement` instances with lengths greater than + /// ``length`` are invalidated by calling this. + /// + /// The UUIDs of :class:`.ParameterVectorElement` instances that are deleted or created by this + /// method are stable; the UUIDs are always derived as index offsets from the vector's UUID. + fn resize(&mut self, length: usize) { + // This can actually allow existing Rust-space `Symbol` objects derived from the prior + // `SymbolVector` to become out of sync with Python space, because we replace the internal + // vector here rather than propagating it through everywhere. + // + // This whole method is not ideal, but it has very limited uses remaining in + // `BlueprintCircuit`, which is deprecated pending removal in Qiskit 3.0. + _ = self.params.take(); + self.base.len.store(length, atomic::Ordering::Relaxed) + } +} + /// Try to extract a Uuid from a Python object, which could be a Python UUID or int. -fn uuid_from_py(py: Python<'_>, uuid: Option>) -> PyResult> { - if let Some(val) = uuid { - // construct from u128 - let as_u128 = if let Ok(as_u128) = val.extract::(py) { - as_u128 - // construct from Python UUID type - } else if val.bind(py).is_exact_instance(UUID.get_bound(py)) { - val.getattr(py, "int")?.extract::(py)? - // invalid format - } else { - return Err(PyTypeError::new_err("not a UUID!")); - }; - Ok(Some(Uuid::from_u128(as_u128))) +fn uuid_from_py(uuid: Bound) -> PyResult { + let int = if uuid.is_exact_instance(UUID.get_bound(uuid.py())) { + uuid.getattr("int")? } else { - Ok(None) - } + uuid + }; + Ok(Uuid::from_u128(int.extract::()?)) } /// Convert a Rust Uuid object to a Python UUID object. -fn uuid_to_py(py: Python<'_>, uuid: Uuid) -> PyResult> { +fn uuid_to_py(py: Python<'_>, uuid: Uuid) -> PyResult> { let uuid = uuid.as_u128(); let kwargs = [("int", uuid)].into_py_dict(py)?; - Ok(UUID.get_bound(py).call((), Some(&kwargs))?.unbind()) -} - -/// Extract a [Symbol] for a Python object, which could either be a Parameter or a -/// ParameterVectorElement. -fn symbol_from_py_parameter(param: &Bound<'_, PyAny>) -> PyResult { - if let Ok(element) = param.extract::() { - Ok(element.symbol.clone()) - } else if let Ok(parameter) = param.extract::() { - Ok(parameter.symbol.clone()) - } else { - Err(PyValueError::new_err("Could not extract parameter")) - } + UUID.get_bound(py).call((), Some(&kwargs)) } /// A singular parameter value used for QPY serialization. This covers anything @@ -1872,17 +1925,9 @@ pub enum ParameterValueType { Float(f64), Complex(Complex64), Parameter(PyParameter), - VectorElement(PyParameterVectorElement), } impl ParameterValueType { - fn from_symbol(symbol: Symbol) -> Self { - if symbol.index.is_some() { - Self::VectorElement(PyParameterVectorElement { symbol }) - } else { - Self::Parameter(PyParameter { symbol }) - } - } fn extract_from_expr(expr: &SymbolExpr) -> Option { if let Some(value) = expr.eval(true) { match value { @@ -1891,7 +1936,7 @@ impl ParameterValueType { Value::Complex(c) => Some(ParameterValueType::Complex(c)), } } else if let SymbolExpr::Symbol(symbol) = expr { - Some(Self::from_symbol(symbol.as_ref().clone())) + Some(Self::Parameter(PyParameter(symbol.clone()))) } else { // ParameterExpressions have the value None, as they must be constructed None @@ -1902,12 +1947,8 @@ impl ParameterValueType { impl From for ParameterExpression { fn from(value: ParameterValueType) -> Self { match value { - ParameterValueType::Parameter(param) => { - let expr = SymbolExpr::Symbol(Arc::new(param.symbol)); - Self::from_symbol_expr(expr) - } - ParameterValueType::VectorElement(param) => { - let expr = SymbolExpr::Symbol(Arc::new(param.symbol)); + ParameterValueType::Parameter(PyParameter(symbol)) => { + let expr = SymbolExpr::Symbol(symbol); Self::from_symbol_expr(expr) } ParameterValueType::Int(i) => { @@ -2136,8 +2177,7 @@ fn qpy_replay_inner( // add the expression to the replay match lhs_value { - Some(ParameterValueType::Parameter(_)) - | Some(ParameterValueType::VectorElement(_)) => { + Some(ParameterValueType::Parameter(_)) => { // For non-commutative operations (SUB, DIV, POW): if LHS is a Parameter and RHS is // an expression (None), we need to use reverse operations (RSUB, RDIV, RPOW) let op = match op { diff --git a/crates/circuit/src/parameter/symbol_expr.rs b/crates/circuit/src/parameter/symbol_expr.rs index de1a988d0c65..80b271f0fba3 100644 --- a/crates/circuit/src/parameter/symbol_expr.rs +++ b/crates/circuit/src/parameter/symbol_expr.rs @@ -11,53 +11,58 @@ // that they have been altered from the originals. use hashbrown::HashMap; -use pyo3::IntoPyObjectExt; -use pyo3::exceptions::PyValueError; use std::borrow::Cow; -use std::cmp::Ordering; -use std::cmp::PartialOrd; +use std::cmp::{Ord, Ordering, PartialOrd}; use std::convert::From; use std::fmt; -use std::hash::Hash; +use std::hash::{Hash, Hasher}; use std::ops::{Add, Div, Mul, Neg, Sub}; -use std::sync::Arc; +use std::sync::{Arc, atomic}; use uuid::Uuid; use num_complex::Complex64; use pyo3::prelude::*; use crate::parameter::parameter_expression::PyParameter; -use crate::parameter::parameter_expression::PyParameterVectorElement; // epsilon for SymbolExpr is heuristically defined pub const SYMEXPR_EPSILON: f64 = f64::EPSILON * 8.0; #[derive(Debug, Clone)] -pub struct Symbol { - pub name: String, // the name of the symbol - pub uuid: Uuid, // the unique identifier - pub index: Option, // an optional index, if part of a vector - pub vector: Option>, // Python only: a reference to the vector, if it is an element +pub enum Symbol { + Standalone { + name: String, + uuid: Uuid, + }, + Element { + index: usize, + base: Arc, + }, } - -/// Custom implementations of Eq, PartialEq, PartialOrd and Hash to ignore the ``vector`` field +impl PartialOrd for Symbol { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} +impl Ord for Symbol { + fn cmp(&self, other: &Self) -> Ordering { + self.ord_key().cmp(&other.ord_key()) + } +} +// These derived versions of `PartialEq` and `Hash` are because `SymbolVector` is (unfortunately) +// internally mutable to support the Python-space `ParameterVector.resize` propagating its mutations +// through to the backreferences, but happily Python-space never actually checked if the +// `ParameterVector` backrefs inside `ParameterVectorElement` were equal, so we don't either. Once +// `ParameterVector.resize` is gone, we can make this more sane by deriving `Eq` and `Hash`. impl PartialEq for Symbol { fn eq(&self, other: &Self) -> bool { - self.name == other.name && self.uuid == other.uuid && self.index == other.index + self.ord_key() == other.ord_key() } } - impl Eq for Symbol {} - -impl PartialOrd for Symbol { - fn partial_cmp(&self, other: &Self) -> Option { - (&self.name, self.uuid, self.index).partial_cmp(&(&other.name, other.uuid, other.index)) - } -} - impl Hash for Symbol { - fn hash(&self, state: &mut H) { - (&self.name, self.uuid, self.index).hash(state); + fn hash(&self, state: &mut H) { + self.ord_key().hash(state) } } @@ -65,80 +70,127 @@ impl<'a, 'py> FromPyObject<'a, 'py> for Symbol { type Error = PyErr; fn extract(ob: Borrowed<'a, 'py, PyAny>) -> Result { - if let Ok(py_vector_element) = ob.extract::() { - Ok(py_vector_element.symbol().clone()) - } else { - ob.extract::() - .map(|ob| ob.symbol().clone()) - .map_err(PyErr::from) - } + Ok(Self::clone(&ob.cast::()?.borrow().0)) } } impl<'py> IntoPyObject<'py> for Symbol { - type Target = PyAny; // to cover PyParameter and PyParameterVectorElement - type Output = Bound<'py, Self::Target>; - type Error = PyErr; + type Target = >::Target; + type Output = >::Output; + type Error = >::Error; fn into_pyobject(self, py: Python<'py>) -> Result { - if self.is_vector_element() { - Py::new(py, PyParameterVectorElement::from_symbol(self.clone()))?.into_bound_py_any(py) - } else { - Py::new(py, PyParameter::from_symbol(self.clone()))?.into_bound_py_any(py) - } + PyParameter(Arc::new(self)).into_pyobject(py) } } impl Symbol { - pub fn new(name: &str, uuid: Option, index: Option) -> Self { - Self { - name: name.to_string(), - uuid: uuid.unwrap_or(Uuid::new_v4()), - index, - vector: None, // Python only - } + /// Create a new standalone (not in a vector) symbol. + pub fn standalone(name: String, uuid: Option) -> Self { + let uuid = uuid.unwrap_or_else(Uuid::new_v4); + Self::Standalone { name, uuid } } - /// In addition to ``new``, this also takes a vector. - pub fn py_new( - name: &str, - uuid: Option, - index: Option, - vector: Option>, - ) -> PyResult { - if index.is_some() != vector.is_some() { - return Err(PyValueError::new_err( - "Either both of vector and index must be provided, or neither of them", - )); + /// The base name of the symbol, without any vector-index suffix. + /// + /// This does not include the vector name. Use [`fullname`] if you need that for display. + pub fn name(&self) -> &str { + match self { + Self::Standalone { name, uuid: _ } => name.as_str(), + Self::Element { base, index: _ } => base.name.as_str(), } - - Ok(Self { - name: name.to_string(), - uuid: uuid.map_or_else(Uuid::new_v4, Uuid::from_u128), - index, - vector, - }) } - pub fn name(&self) -> &str { - &self.name + pub fn fullname(&self) -> Cow<'_, str> { + match self { + Self::Standalone { name, uuid: _ } => Cow::Borrowed(name.as_str()), + Self::Element { base, index } => Cow::Owned(format!("{}[{}]", &base.name, index)), + } } - pub fn fullname(&self) -> Cow<'_, str> { - self.index - .map(|i| Cow::Owned(format!("{}[{}]", &self.name, i))) - .unwrap_or(Cow::Borrowed(&self.name)) + pub fn uuid(&self) -> Uuid { + match self { + Self::Standalone { uuid, name: _ } => *uuid, + Self::Element { base, index } => Uuid::from_u128(base.uuid.as_u128() + *index as u128), + } } pub fn repr(&self, with_uuid: bool) -> String { if with_uuid { - format!("{}_{}", self.fullname(), self.uuid.as_u128()) + format!("{}_{}", self.fullname(), self.uuid().as_u128()) } else { self.fullname().into_owned() } } - pub fn is_vector_element(&self) -> bool { - matches!((&self.index, &self.vector), (Some(_), Some(_))) + + /// The key object to use for all comparison operations with this object. + pub fn ord_key(&self) -> impl ::std::cmp::Ord + ::std::hash::Hash + use<'_> { + ( + // This is the base name, without the vector-index appended, because we use those + // numerically. + self.name(), + match self { + Self::Standalone { .. } => None, + // Everything about `base` that we actually use is inside `name` and `uuid`; we've + // never actually required that the backrefs inside vector elements are to equal + // vectors (!). While `ParameterExpression.resize` still exists, this actually is + // useful for us: we can maintain equality checks through serialisation roundtrips. + Self::Element { index, base: _ } => Some(index), + }, + self.uuid(), + ) + } +} + +/// Vector of multiple parameters. +/// +/// This makes creating large numbers of parameters much more efficient; we no longer need to +/// allocate individual strings and UUIDs for each symbol, but instead derive them all from the +/// shared base. +/// +/// This object is intended to be used almost exclusively behind an `Arc`, since all derived +/// symbols from it need an `Arc` backreference. +#[derive(Debug)] +pub struct SymbolVector { + pub name: String, + pub uuid: Uuid, + /// Length of the vector. + /// + /// This is `AtomicUsize` to support the (unfortunate) Python-space method + /// `ParameterVector.resize`. Once that method is deprecated and removed, this can become a + /// regular `usize` (and we'll be able to derive `Hash`, if we want). + pub len: atomic::AtomicUsize, +} +impl SymbolVector { + pub fn new(name: String, len: usize) -> Arc { + Arc::new(Self { + name, + uuid: Uuid::new_v4(), + len: len.into(), + }) + } + + pub fn get(self: &Arc, index: usize) -> Option { + (index < self.len.load(atomic::Ordering::Relaxed)).then(|| Symbol::Element { + index, + base: Arc::clone(self), + }) + } + + pub fn iter(self: &Arc) -> impl ExactSizeIterator { + (0..self.len.load(atomic::Ordering::Relaxed)).map(|index| { + self.get(index) + .expect("the iterator is only over valid values") + }) + } +} +impl Clone for SymbolVector { + fn clone(&self) -> Self { + Self { + name: self.name.clone(), + uuid: self.uuid, + len: self.len.load(atomic::Ordering::Relaxed).into(), + } } } @@ -2896,12 +2948,6 @@ impl PartialOrd for SymbolExpr { } } -impl From<&str> for SymbolExpr { - fn from(v: &str) -> Self { - SymbolExpr::Symbol(Arc::new(Symbol::new(v, None, None))) - } -} - impl fmt::Display for Value { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( @@ -3346,7 +3392,7 @@ impl Value { SymbolExpr::Value(Value::Real(c.re)), _mul( SymbolExpr::Value(Value::Real(c.im)), - SymbolExpr::Symbol(Arc::new(Symbol::new("I", None, None))), + SymbolExpr::Symbol(Arc::new(Symbol::standalone("I".to_owned(), None))), ), ), _ => SymbolExpr::Value(*self), diff --git a/crates/circuit/src/parameter/symbol_parser.rs b/crates/circuit/src/parameter/symbol_parser.rs index fff56f6a3cc8..f52b68c104b7 100644 --- a/crates/circuit/src/parameter/symbol_parser.rs +++ b/crates/circuit/src/parameter/symbol_parser.rs @@ -42,29 +42,21 @@ fn parse_imaginary_value(s: &str) -> IResult<&str, SymbolExpr, VerboseError<&str .parse(s) } -fn parse_symbol_string(s: &str) -> IResult<&str, &str, VerboseError<&str>> { - recognize(pair( - alt((alpha1, tag("_"), tag("\\"), tag("$"))), - many0_count(alt((alphanumeric1, tag("_"), tag("\\"), tag("$")))), - )) - .parse(s) -} - // parse string as symbol // symbol starting with alphabet and can contain numbers and '_', '\', '$', '[', ']' fn parse_symbol(s: &str) -> IResult<&str, SymbolExpr, VerboseError<&str>> { - ( - parse_symbol_string, - opt(delimited(char('['), digit1, char(']'))), + // At this point, we just consume the entire name string, regardless of index; it's up to a + // "name map" later to replace names with complete `Symbol` implementations, since the string + // form doesn't carry sufficient information to correctly assign UUIDs or base vectors. + recognize( + pair( + alt((alpha1, tag("_"), tag("\\"), tag("$"))), + many0_count(alt((alphanumeric1, tag("_"), tag("\\"), tag("$")))), + ) + .and(opt(delimited(char('['), digit1, char(']')))), ) - .map_res(|(v, array_idx)| -> Result<_, &str> { - let index = array_idx - .map(|i| i.parse::()) - .transpose() - .map_err(|_| "index out of bounds")?; - Ok(SymbolExpr::Symbol(Arc::new(Symbol::new(v, None, index)))) - }) - .parse(s) + .map(|v: &str| SymbolExpr::Symbol(Arc::new(Symbol::standalone(v.to_owned(), None)))) + .parse(s) } // parse unary operations diff --git a/crates/circuit/src/parameter_table.rs b/crates/circuit/src/parameter_table.rs index a3b7d0347627..a34cb8291e94 100644 --- a/crates/circuit/src/parameter_table.rs +++ b/crates/circuit/src/parameter_table.rs @@ -75,12 +75,12 @@ impl ParameterUuid { pub fn from_parameter(ob: &Bound) -> PyResult { let uuid = if let Ok(param) = ob.cast::() { // this downcast should cover both PyParameterVectorElement and PyParameter - param.borrow().symbol().uuid.as_u128() + param.borrow().0.uuid().as_u128() } else if let Ok(expr) = ob.cast::() { let expr_borrowed = expr.borrow(); // We know the ParameterExpression is in fact representing a single Symbol let symbol = &expr_borrowed.inner.try_to_symbol()?; - symbol.uuid.as_u128() + symbol.uuid().as_u128() } else { return Err(PyTypeError::new_err( "Could not downcast to Parameter or Expression (that equals a symbol)", @@ -91,7 +91,7 @@ impl ParameterUuid { } pub fn from_symbol(symbol: &Symbol) -> Self { - Self(symbol.uuid.as_u128()) + Self(symbol.uuid().as_u128()) } } @@ -216,12 +216,7 @@ impl ParameterTable { /// Get the sorted order of the `ParameterTable`. This does not access the cache. fn sorted_order(&self) -> Vec { let mut out = self.by_uuid.keys().copied().collect::>(); - out.sort_unstable_by_key(|uuid| { - let info = &self.by_uuid[uuid]; - let index = info.symbol.index.unwrap_or(0); - let name = info.symbol.name(); - (name, index) - }); + out.sort_unstable_by_key(|uuid| self.by_uuid[uuid].symbol.ord_key()); out } diff --git a/crates/qasm3/src/exporter.rs b/crates/qasm3/src/exporter.rs index fc9eaa5925b9..3f5181539496 100644 --- a/crates/qasm3/src/exporter.rs +++ b/crates/qasm3/src/exporter.rs @@ -1307,9 +1307,8 @@ impl<'a> QASM3Builder { .map(|i| { let name = format!("{}_{}", self._gate_param_prefix, i); // TODO this need to be achievable more easily - let symbol = symbol_expr::Symbol::new(name.as_str(), None, None); - let symbol_expr = symbol_expr::SymbolExpr::Symbol(Arc::new(symbol)); - let expr = ParameterExpression::from_symbol_expr(symbol_expr); + let symbol = symbol_expr::Symbol::standalone(name, None); + let expr = ParameterExpression::from_symbol(symbol); Param::ParameterExpression(Arc::new(expr)) }) .collect(); diff --git a/crates/qpy/src/circuit_reader.rs b/crates/qpy/src/circuit_reader.rs index 969602170fdf..3b42eec894e2 100644 --- a/crates/qpy/src/circuit_reader.rs +++ b/crates/qpy/src/circuit_reader.rs @@ -47,7 +47,7 @@ use qiskit_circuit::operations::{ StandardInstruction, }; use qiskit_circuit::packed_instruction::{PackedInstruction, PackedOperation}; -use qiskit_circuit::parameter::parameter_expression::ParameterExpression; +use qiskit_circuit::parameter::parameter_expression::{ParameterExpression, PyParameter}; use qiskit_circuit::var_stretch_container::{StretchType, VarType}; use qiskit_circuit::{Block, classical, imports}; use qiskit_circuit::{Clbit, Qubit}; @@ -298,14 +298,10 @@ pub fn instruction_values_to_params( match value { GenericValue::Float64(float) => Ok(Param::Float(float)), GenericValue::ParameterExpression(exp) => Ok(Param::ParameterExpression(exp)), - GenericValue::ParameterExpressionSymbol(symbol) => { + GenericValue::ParameterExpressionSymbol(symbol) + | GenericValue::ParameterExpressionVectorSymbol(symbol) => { Ok(Param::ParameterExpression(Arc::new( - ParameterExpression::from_symbol(symbol), - ))) - } - GenericValue::ParameterExpressionVectorSymbol(symbol) => { - Ok(Param::ParameterExpression(Arc::new( - ParameterExpression::from_symbol(symbol), + ParameterExpression::from_arc_symbol(symbol), ))) } _ => Ok(Param::Obj(py_convert_from_generic_value(&value)?)), @@ -604,7 +600,9 @@ fn unpack_control_flow( } let collection = unpack_for_collection(&collection_value_pack)?; let loop_param = match loop_param_value_pack { - GenericValue::ParameterExpressionSymbol(symbol) => Some(symbol), + GenericValue::ParameterExpressionSymbol(symbol) => { + Some(Arc::unwrap_or_clone(symbol)) + } _ => None, }; ControlFlow::ForLoop { @@ -1234,8 +1232,8 @@ fn deserialize_pauli_evolution_gate( let py_time: Py = match time { GenericValue::Float64(value) => value.into_py_any(py)?, GenericValue::ParameterExpression(exp) => exp.as_ref().clone().into_py_any(py)?, - GenericValue::ParameterExpressionVectorSymbol(symbol) => symbol.into_py_any(py)?, - GenericValue::ParameterExpressionSymbol(symbol) => symbol.into_py_any(py)?, + GenericValue::ParameterExpressionVectorSymbol(symbol) + | GenericValue::ParameterExpressionSymbol(symbol) => PyParameter(symbol).into_py_any(py)?, _ => return Err(QpyError::InvalidParameter( "Pauli Evolution Gate 'time' parameter should be either float or parameter expression" .to_string(), diff --git a/crates/qpy/src/circuit_writer.rs b/crates/qpy/src/circuit_writer.rs index 3332c1978f6d..781ca26bc6d3 100644 --- a/crates/qpy/src/circuit_writer.rs +++ b/crates/qpy/src/circuit_writer.rs @@ -469,7 +469,7 @@ fn pack_control_flow_inst( let collection_value = pack_for_collection(&collection); let loop_param_value = match loop_param { None => GenericValue::Null, - Some(symbol) => GenericValue::ParameterExpressionSymbol(symbol), + Some(symbol) => GenericValue::ParameterExpressionSymbol(symbol.into()), }; let mut params = Vec::new(); params.push(pack_generic_value(&collection_value, qpy_data)?); diff --git a/crates/qpy/src/params.rs b/crates/qpy/src/params.rs index 446b3853e00f..2ca76af551b9 100644 --- a/crates/qpy/src/params.rs +++ b/crates/qpy/src/params.rs @@ -12,14 +12,12 @@ use binrw::Endian; use num_complex::Complex64; use pyo3::prelude::*; -use pyo3::types::IntoPyDict; -use qiskit_circuit::imports; use qiskit_circuit::operations::Param; use qiskit_circuit::parameter::parameter_expression::{ OPReplay, ParameterExpression, ParameterValueType, }; -use qiskit_circuit::parameter::symbol_expr::Symbol; -use std::sync::Arc; +use qiskit_circuit::parameter::symbol_expr::{Symbol, SymbolVector}; +use std::sync::{Arc, atomic}; use uuid::Uuid; use crate::bytes::Bytes; @@ -31,7 +29,7 @@ use crate::value::{ pack_generic_value, serialize, }; use binrw::binrw; -use hashbrown::HashMap; +use hashbrown::hash_map::HashMap; // The various values of values that can exist in a parameter expression node // This data is stored inside the parent of the node, not in the node itself @@ -154,29 +152,28 @@ pub(crate) fn pack_parameter_expression( fn pack_symbol_table_element( symbol: &Symbol, ) -> Result { - let value_data = Bytes::new(); // this was used only when packing symbol tables related to substitution commands and no longer relevant - if symbol.is_vector_element() { - let value_key = ValueType::ParameterVector; - let symbol_data = pack_parameter_vector(symbol)?; - let symbol_pack = formats::ParameterExpressionParameterVectorSymbolPack { - value_key, - value_data, - symbol_data, - }; - Ok(formats::ParameterExpressionSymbolPack::ParameterVector( - symbol_pack, - )) - } else { - let value_key = ValueType::Parameter; - let symbol_data = pack_symbol(symbol); - let symbol_pack = formats::ParameterExpressionParameterSymbolPack { - value_key, - value_data, - symbol_data, - }; - Ok(formats::ParameterExpressionSymbolPack::Parameter( - symbol_pack, - )) + // This was used only when packing symbol tables related to substitution commands and no longer + // relevant. + let value_data = Bytes::new(); + match symbol { + Symbol::Standalone { .. } => { + let pack = formats::ParameterExpressionParameterSymbolPack { + value_key: ValueType::Parameter, + value_data, + symbol_data: pack_symbol(symbol), + }; + Ok(formats::ParameterExpressionSymbolPack::Parameter(pack)) + } + Symbol::Element { .. } => { + let pack = formats::ParameterExpressionParameterVectorSymbolPack { + value_key: ValueType::ParameterVector, + value_data, + symbol_data: pack_parameter_vector(symbol)?, + }; + Ok(formats::ParameterExpressionSymbolPack::ParameterVector( + pack, + )) + } } } @@ -232,12 +229,10 @@ fn pack_parameter_replay_entry( Bytes::from(val).try_to_16_byte_slice()?, ), ParameterValueType::Parameter(parameter) => { - (ParameterType::Parameter, *parameter.symbol.uuid.as_bytes()) + // We don't distinguish between `Parameter` and `ParameterVectorElement` in the QPY + // format here, because we only use the UUID as a lookup anyway. + (ParameterType::Parameter, *parameter.0.uuid().as_bytes()) } - ParameterValueType::VectorElement(element) => ( - ParameterType::Parameter, // Python QPY expects Parameter, not ParameterVector - *element.symbol.uuid.as_bytes(), - ), }) } @@ -283,7 +278,7 @@ pub(crate) fn unpack_parameter_expression( return Ok(map); } }; - map.insert(symbol.uuid, symbol); + map.insert(symbol.uuid(), symbol); Ok(map) }, )?; @@ -354,7 +349,7 @@ pub(crate) fn unpack_parameter_expression( match value { GenericValue::ParameterExpressionSymbol(sym) | GenericValue::ParameterExpressionVectorSymbol(sym) => { - Ok(ParameterExpression::from_symbol(sym)) + Ok(ParameterExpression::from_arc_symbol(sym)) } GenericValue::ParameterExpression(expr) => Ok((*expr).clone()), GenericValue::Int64(_) | GenericValue::Float64(_) | GenericValue::Complex64(_) => { @@ -476,101 +471,68 @@ pub(crate) fn unpack_parameter_expression( } pub(crate) fn pack_symbol(symbol: &Symbol) -> formats::ParameterSymbolPack { - let uuid = *symbol.uuid.as_bytes(); - let name = symbol.name.clone(); + let uuid = *symbol.uuid().as_bytes(); + let name = symbol.name().to_owned(); formats::ParameterSymbolPack { uuid, name } } pub(crate) fn unpack_symbol(parameter_pack: &formats::ParameterSymbolPack) -> Symbol { let name = parameter_pack.name.clone(); let uuid = Uuid::from_bytes(parameter_pack.uuid); - Symbol { - name, - uuid, - index: None, - vector: None, - } + Symbol::Standalone { name, uuid } } -// currently, the only way to extract the length of the vector the symbol belongs to -// is via python space since the vector is stored as a python reference in the symbol pub(crate) fn pack_parameter_vector( symbol: &Symbol, ) -> Result { - let vector_size = Python::attach(|py| -> Result<_, QpyError> { - match &symbol.vector { - None => Err(QpyError::ConversionError( - "No vector data for parameter vector element".to_string(), - )), - Some(vector) => Ok(vector.bind(py).call_method0("__len__")?.extract()?), - } - })?; - let index = match symbol.index { - None => { - return Err(QpyError::ConversionError( - "No index data for parameter vector element".to_string(), - )); - } - Some(index_value) => index_value as u64, + let Symbol::Element { index, base } = symbol else { + return Err(QpyError::ConversionError( + "internal logic error: attempted to pack a standalone symbol as a vector element" + .to_owned(), + )); }; Ok(formats::ParameterVectorElementPack { - vector_size, - uuid: *symbol.uuid.as_bytes(), - index, - name: symbol.name.clone(), + vector_size: base.len.load(atomic::Ordering::Relaxed) as u64, + uuid: *symbol.uuid().as_bytes(), + index: *index as u64, + name: base.name.clone(), }) } -// parameter vector symbols are currently much more tricky than standalone symbols -// since we don't have a rust-space concept of ParameterVector; it is a pure python object -// which we must manage while creating its elements. Moreover, the vector itself is not stored anywhere -// so we need to create it in an ad-hoc fashion as we encounter its elements during the parsing of the -// qpy file. In particular we need to keep our qpy_data nearby so we can update the vector list as needed -// and we must use python calls to create and modify the python-space ParameterVector +/// Unpack a `ParameterVectorElement` into a `Symbol`. +/// +/// Actually, the majority of the work we have to do here is unpacking the underlying _vector_ and +/// ensuring it is consistent with any other definitions of this vector that we've seen. pub(crate) fn unpack_parameter_vector( - parameter_vector_pack: &formats::ParameterVectorElementPack, + pack: &formats::ParameterVectorElementPack, qpy_data: &mut QPYReadData, ) -> Result { - let name = parameter_vector_pack.name.clone(); - let uuid = Uuid::from_bytes(parameter_vector_pack.uuid); - let index = parameter_vector_pack.index as u32; // sadly, the `Symbol` class does not conform to the qpy u64 format - // we have extracted the rust-space data, but now we must deal with the python-space vector class - - // first get the uuid for the vector's "root" (it's first element) - // we rely on the convention that the uuid's for the vector elements are sequential - let root_uuid_int = uuid.as_u128() - (index as u128); - let root_uuid = Uuid::from_bytes(root_uuid_int.to_be_bytes()); - - let vector = Python::attach(|py| -> Result<_, QpyError> { - // we use python-space to interface with the ParameterVector data - let vector = match qpy_data.vectors.get(&root_uuid) { - Some(value) => value, - None => Python::attach(|py| -> Result<_, QpyError> { - // we use python-space to create a new parameter vector - let py_uuid = { - let kwargs = [("int", root_uuid_int)].into_py_dict(py)?; - py.import("uuid")? - .getattr("UUID")? - .call((), Some(&kwargs))? - }; - let vector = imports::PARAMETER_VECTOR - .get_bound(py) - .call1((name.clone(), parameter_vector_pack.vector_size, py_uuid))? - .unbind(); - qpy_data.vectors.insert(root_uuid, vector); - qpy_data.vectors.get(&root_uuid).ok_or_else(|| { - QpyError::MissingData("Parameter vector creation failed".to_string()) - }) - })?, - }; - Ok(vector.clone_ref(py)) - })?; - - Ok(Symbol { - name, - uuid, - index: Some(index), - vector: Some(vector), + // Historical versions of Qiskit assigned independent UUIDs to every element of a vector. + // Modern Qiskit doesn't even permit this representation; elements' UUIDs are offset from the + // base vector's. With certain payloads from very old Qiskit versions (pre Terra 0.25), this + // would cause elements to disagree on the vectors that own them, but that largely shouldn't be + // observable, and at the time of writing (2026-05-19), we don't handle old enough QPY versions + // in Rust space for it to trigger. + let uuid = Uuid::from_u128(u128::from_be_bytes(pack.uuid) - (pack.index as u128)); + let vector = qpy_data.vectors.entry(uuid).or_insert_with(|| { + Arc::new(SymbolVector { + name: pack.name.clone(), + uuid, + len: (pack.vector_size as usize).into(), + }) + }); + let vector_len = vector.len.load(atomic::Ordering::Relaxed); + if vector.name != pack.name || vector_len != pack.vector_size as usize { + return Err(QpyError::InvalidParameter(format!( + "'{}[{}]' has a base vector ('{}[{}]') that disagrees with another ('{}[{}]')", + &pack.name, pack.index, &pack.name, pack.vector_size, &vector.name, vector_len, + ))); + } + vector.get(pack.index as usize).ok_or_else(|| { + QpyError::InvalidParameter(format!( + "index {} is out of range for vector '{}[{}]'", + pack.index, &vector.name, vector_len + )) }) } @@ -580,10 +542,12 @@ pub(crate) fn pack_param_expression( ) -> Result { // 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() { - match symbol.vector { - None => pack_generic_value(&GenericValue::ParameterExpressionSymbol(symbol), qpy_data), - Some(_) => pack_generic_value( + if let Ok(symbol) = exp.try_to_symbol().map(Arc::new) { + match &*symbol { + Symbol::Standalone { .. } => { + pack_generic_value(&GenericValue::ParameterExpressionSymbol(symbol), qpy_data) + } + Symbol::Element { .. } => pack_generic_value( &GenericValue::ParameterExpressionVectorSymbol(symbol), qpy_data, ), @@ -623,11 +587,11 @@ pub(crate) fn generic_value_to_param(value: &GenericValue) -> Result Ok(Param::Float(*float_val)), GenericValue::ParameterExpressionSymbol(symbol) => { - let parameter_expression = ParameterExpression::from_symbol(symbol.clone()); + let parameter_expression = ParameterExpression::from_arc_symbol(symbol.clone()); Ok(Param::ParameterExpression(Arc::new(parameter_expression))) } GenericValue::ParameterExpressionVectorSymbol(symbol) => { - let parameter_expression = ParameterExpression::from_symbol(symbol.clone()); + let parameter_expression = ParameterExpression::from_arc_symbol(symbol.clone()); Ok(Param::ParameterExpression(Arc::new(parameter_expression))) } GenericValue::ParameterExpression(exp) => Ok(Param::ParameterExpression(exp.clone())), diff --git a/crates/qpy/src/py_methods.rs b/crates/qpy/src/py_methods.rs index 39381c212833..e25b6832ba65 100644 --- a/crates/qpy/src/py_methods.rs +++ b/crates/qpy/src/py_methods.rs @@ -27,9 +27,7 @@ use qiskit_circuit::classical; use qiskit_circuit::imports; use qiskit_circuit::operations::{Operation, OperationRef, PyInstruction, PyOpKind, PyRange}; use qiskit_circuit::packed_instruction::PackedOperation; -use qiskit_circuit::parameter::parameter_expression::{ - PyParameter, PyParameterExpression, PyParameterVectorElement, -}; +use qiskit_circuit::parameter::parameter_expression::{PyParameter, PyParameterExpression}; use qiskit_quantum_info::sparse_observable::PySparseObservable; use uuid::Uuid; @@ -363,16 +361,18 @@ pub(crate) fn py_convert_to_generic_value( ValueType::Null => Ok(GenericValue::Null), ValueType::Parameter => Ok(GenericValue::ParameterExpressionSymbol( py_object - .extract::() - .map_err(|e| QpyError::from(PyErr::from(e)))? - .symbol() + .cast::() + .map_err(PyErr::from)? + .borrow() + .0 .clone(), )), ValueType::ParameterVector => Ok(GenericValue::ParameterExpressionVectorSymbol( py_object - .extract::() - .map_err(|e| QpyError::from(PyErr::from(e)))? - .symbol() + .cast::() + .map_err(PyErr::from)? + .borrow() + .0 .clone(), )), ValueType::ParameterExpression => Ok(GenericValue::ParameterExpression(Arc::new( @@ -434,11 +434,9 @@ pub(crate) fn py_convert_from_generic_value(value: &GenericValue) -> Result Ok(exp.clone().into_py_any(py)?), GenericValue::CaseDefault => Ok(imports::CASE_DEFAULT.get(py).clone()), GenericValue::Null => Ok(py.None()), - GenericValue::ParameterExpressionSymbol(symbol) => { - Ok(symbol.clone().into_py_any(py)?) - } - GenericValue::ParameterExpressionVectorSymbol(symbol) => { - Ok(symbol.clone().into_py_any(py)?) + GenericValue::ParameterExpressionSymbol(symbol) + | GenericValue::ParameterExpressionVectorSymbol(symbol) => { + Ok(PyParameter(symbol.clone()).into_py_any(py)?) } GenericValue::ParameterExpression(exp) => Ok(exp.as_ref().clone().into_py_any(py)?), GenericValue::Circuit(py_object) => Ok(py_object.clone()), diff --git a/crates/qpy/src/value.rs b/crates/qpy/src/value.rs index fecff0c8cdb6..c2062b7ec7d5 100644 --- a/crates/qpy/src/value.rs +++ b/crates/qpy/src/value.rs @@ -28,7 +28,7 @@ use qiskit_circuit::duration::Duration; use qiskit_circuit::operations::{ForCollection, OperationRef, PyInstruction, PyOpKind, PyRange}; use qiskit_circuit::packed_instruction::PackedOperation; use qiskit_circuit::parameter::parameter_expression::ParameterExpression; -use qiskit_circuit::parameter::symbol_expr::Symbol; +use qiskit_circuit::parameter::symbol_expr::{Symbol, SymbolVector}; use qiskit_circuit::{Clbit, imports}; use crate::annotations::AnnotationHandler; @@ -125,7 +125,7 @@ pub struct QPYReadData<'a> { pub use_symengine: bool, pub standalone_vars: HashMap, pub standalone_stretches: HashMap, - pub vectors: HashMap>, // Parameter expression vectors, which are a python-only elements for now + pub vectors: HashMap>, pub annotation_handler: AnnotationHandler<'a>, } @@ -305,8 +305,8 @@ pub enum GenericValue { Range(PyRange), Tuple(Vec), NumpyObject(Bytes), - ParameterExpressionSymbol(Symbol), - ParameterExpressionVectorSymbol(Symbol), + ParameterExpressionSymbol(Arc), + ParameterExpressionVectorSymbol(Arc), ParameterExpression(Arc), String(String), Duration(Duration), @@ -496,13 +496,13 @@ pub(crate) fn load_value( ValueType::Parameter => { let (parameter_pack, _) = deserialize::(bytes)?; let symbol = unpack_symbol(¶meter_pack); - Ok(GenericValue::ParameterExpressionSymbol(symbol)) + Ok(GenericValue::ParameterExpressionSymbol(symbol.into())) } ValueType::ParameterVector => { let (parameter_vector_element_pack, _) = deserialize::(bytes)?; let symbol = unpack_parameter_vector(¶meter_vector_element_pack, qpy_data)?; - Ok(GenericValue::ParameterExpressionVectorSymbol(symbol)) + Ok(GenericValue::ParameterExpressionVectorSymbol(symbol.into())) } ValueType::ParameterExpression => { let (parameter_expression_pack, _) = diff --git a/crates/transpiler/src/passes/basis_translator/compose_transforms.rs b/crates/transpiler/src/passes/basis_translator/compose_transforms.rs index 807b71b0a249..8868e187dd57 100644 --- a/crates/transpiler/src/passes/basis_translator/compose_transforms.rs +++ b/crates/transpiler/src/passes/basis_translator/compose_transforms.rs @@ -19,7 +19,7 @@ use qiskit_circuit::instruction::Parameters; use qiskit_circuit::operations::CustomOperation; use qiskit_circuit::packed_instruction::PackedOperation; use qiskit_circuit::parameter::parameter_expression::ParameterExpression; -use qiskit_circuit::parameter::symbol_expr::Symbol; +use qiskit_circuit::parameter::symbol_expr::SymbolVector; use qiskit_circuit::parameter_table::ParameterUuid; use qiskit_circuit::{ dag_circuit::DAGCircuit, @@ -44,14 +44,11 @@ pub(super) fn compose_transforms<'a>( for (gate_name, gate_num_qubits) in source_basis.iter().cloned() { let num_params = gate_param_counts[&(gate_name.clone(), gate_num_qubits)]; - let placeholder_params: SmallVec<[Param; 3]> = (0..num_params as u32) - .map(|idx| { - Param::ParameterExpression( - ParameterExpression::from_symbol(Symbol::new(&gate_name, None, Some(idx))) - .into(), - ) - }) - .collect(); + let placeholder_params: SmallVec<[Param; 3]> = + SymbolVector::new(gate_name.clone(), num_params) + .iter() + .map(|sym| Param::ParameterExpression(ParameterExpression::from_symbol(sym).into())) + .collect(); let mut dag = DAGCircuit::new(); // Create the mock gate and add to the circuit, use Python if necessary. diff --git a/crates/transpiler/src/standard_equivalence_library.rs b/crates/transpiler/src/standard_equivalence_library.rs index be9569517a1c..bbeb5904d32b 100644 --- a/crates/transpiler/src/standard_equivalence_library.rs +++ b/crates/transpiler/src/standard_equivalence_library.rs @@ -161,8 +161,9 @@ pub fn generate_standard_equivalence_library() -> EquivalenceLibrary { // ┌──────┐ ┌───────┐ // q: ┤ P(ϴ) ├ ≡ q: ┤ U1(ϴ) ├ // └──────┘ └───────┘ - let theta = Arc::new(ParameterExpression::from_symbol(Symbol::new( - "theta", None, None, + let theta = Arc::new(ParameterExpression::from_symbol(Symbol::standalone( + "theta".to_owned(), + None, ))); create_standard_equivalence( StandardGate::Phase, @@ -295,8 +296,9 @@ pub fn generate_standard_equivalence_library() -> EquivalenceLibrary { // ┌────────┐ ┌──────────────────────┐ // q: ┤ R(ϴ,φ) ├ ≡ q: ┤ U(ϴ,φ - π/2,π/2 - φ) ├ // └────────┘ └──────────────────────┘ - let phi = Arc::new(ParameterExpression::from_symbol(Symbol::new( - "phi", None, None, + let phi = Arc::new(ParameterExpression::from_symbol(Symbol::standalone( + "phi".to_owned(), + None, ))); // π/2 let pi_div_2 = Arc::new(ParameterExpression::from_f64(PI / 2.0)); @@ -1913,8 +1915,9 @@ pub fn generate_standard_equivalence_library() -> EquivalenceLibrary { // ┌──────────┐ ┌───────────┐ // q: ┤ U(θ,ϕ,λ) ├ ≡ q: ┤ U3(θ,ϕ,λ) ├ // └──────────┘ └───────────┘ - let lam = Arc::new(ParameterExpression::from_symbol(Symbol::new( - "lam", None, None, + let lam = Arc::new(ParameterExpression::from_symbol(Symbol::standalone( + "lam".to_owned(), + None, ))); create_standard_equivalence( StandardGate::U, @@ -1948,8 +1951,9 @@ pub fn generate_standard_equivalence_library() -> EquivalenceLibrary { // « ┌──────────────────────┐┌─┴─┐┌────────────┐ // «q_1: ┤ U(-θ/2,0,-λ/2 - ϕ/2) ├┤ X ├┤ U(θ/2,ϕ,0) ├ // « └──────────────────────┘└───┘└────────────┘ - let gamma = Arc::new(ParameterExpression::from_symbol(Symbol::new( - "gamma", None, None, + let gamma = Arc::new(ParameterExpression::from_symbol(Symbol::standalone( + "gamma".to_owned(), + None, ))); let lam_plus_phi = Arc::new(lam.add(&phi).unwrap()); let lam_plus_phi_div_2 = Arc::new( @@ -2949,8 +2953,9 @@ pub fn generate_standard_equivalence_library() -> EquivalenceLibrary { // ≡ ┌┴───────┴─┐├───┴┐┌─────────┐└─┬─┘├────────────┤└─┬─┘┌─┴─────┴──┐└──┬──────┬──┘┌─────────┐ // ┤ Rz(-π/2) ├┤ √X ├┤ Rz(π/2) ├──■──┤ Ry(-0.5*θ) ├──■──┤ Rz(-π/2) ├───┤ √Xdg ├───┤ Rz(π/2) ├ // └──────────┘└────┘└─────────┘ └────────────┘ └──────────┘ └──────┘ └─────────┘ - let beta = Arc::new(ParameterExpression::from_symbol(Symbol::new( - "beta", None, None, + let beta = Arc::new(ParameterExpression::from_symbol(Symbol::standalone( + "beta".to_owned(), + None, ))); let neg_beta = Arc::new(beta.mul(&ParameterExpression::from_f64(-1.0)).unwrap()); create_standard_equivalence( diff --git a/crates/transpiler/src/target/mod.rs b/crates/transpiler/src/target/mod.rs index d4a80f6921a2..0e3e091544b8 100644 --- a/crates/transpiler/src/target/mod.rs +++ b/crates/transpiler/src/target/mod.rs @@ -1705,15 +1705,15 @@ mod test { #[test] fn test_invalid_params_instruction() { let params = smallvec![ - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "ϴ", None, None, - )))), - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "φ", None, None, - )))), - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "λ", None, None, - )))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("ϴ".to_owned(), None,) + ))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("φ".to_owned(), None,) + ))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("λ".to_owned(), None,) + ))), ]; let mut target = Target::default(); let result = target.add_instruction( @@ -1737,15 +1737,15 @@ mod test { #[test] fn test_mismatch_params_count_instruction() { let params = smallvec![ - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "ϴ", None, None, - )))), - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "φ", None, None, - )))), - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "λ", None, None, - )))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("ϴ".to_owned(), None,) + ))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("φ".to_owned(), None,) + ))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("λ".to_owned(), None,) + ))), ]; let mut target = Target::default(); let result = target.add_instruction( diff --git a/crates/transpiler/src/transpiler.rs b/crates/transpiler/src/transpiler.rs index d6c229af5c7e..77330147a4d9 100644 --- a/crates/transpiler/src/transpiler.rs +++ b/crates/transpiler/src/transpiler.rs @@ -627,15 +627,15 @@ mod tests { fn build_universal_star_target() -> Target { let mut target = Target::default(); let u_params = Some(Parameters::Params(smallvec![ - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "a", None, None, - )))), - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "b", None, None, - )))), - Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(Symbol::new( - "c", None, None, - )))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("a".to_owned(), None) + ))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("b".to_owned(), None) + ))), + Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol( + Symbol::standalone("c".to_owned(), None) + ))), ])); let props = (0..5) diff --git a/qiskit/circuit/parametervector.py b/qiskit/circuit/parametervector.py index 84c63f3b0de9..6673bd3008ea 100644 --- a/qiskit/circuit/parametervector.py +++ b/qiskit/circuit/parametervector.py @@ -12,128 +12,7 @@ """Parameter Vector Class to simplify management of parameter lists.""" -from uuid import uuid4, UUID +__all__ = ["ParameterVector", "ParameterVectorElement"] -from qiskit._accelerate.circuit import ParameterVectorElement - - -class ParameterVector: - """A container of many related :class:`Parameter` objects. - - This class is faster to construct than constructing many :class:`Parameter` objects - individually, and the individual names of the parameters will all share a common stem (the name - of the vector). For a vector called ``v`` with length 3, the individual elements will have - names ``v[0]``, ``v[1]`` and ``v[2]``. - - The elements of a vector are sorted by the name of the vector, then the numeric value of their - index. - - This class fulfills the :class:`collections.abc.Sequence` interface. - - .. note:: - This class does not implement regular equality, but has historically been allowed to be used - as a dictionary key for :meth:`.QuantumCircuit.assign_parameters`. The class is mutable via - :meth:`resize`, so it generally cannot implement regular equality; for two - :class:`.ParameterVector` objects to compare equal, they must be literally the same Python - instance. - - This restriction does not apply to the individual :class:`.ParameterVectorElement` - instances; these must only match on name, index and UUID. - """ - - __slots__ = ("_name", "_params", "_root_uuid") - - def __init__(self, name: str, length: int = 0, uuid: UUID | None = None): - """ - Args: - name: the base name of the vector - length: the number of elements in the vector - uuid: (optional) the root UUID to use for the vector. This can be used to create a new - vector whose elements will compare equal to a previous vector (such as in a - deserialization process). - """ - self._name = name - self._root_uuid = uuid or uuid4() - root_uuid_int = self._root_uuid.int - self._params = [ - ParameterVectorElement(self, i, UUID(int=root_uuid_int + i)) for i in range(length) - ] - - @property - def name(self): - """The name of the :class:`ParameterVector`.""" - return self._name - - @property - def params(self): - """A list of the contained :class:`ParameterVectorElement` instances. - - It is not safe to mutate this list.""" - return self._params - - @property - def uuid(self) -> UUID: - """Get the root UUID of this vector.""" - return self._root_uuid - - def index(self, value): - """Find the index of a :class:`ParameterVectorElement` within the list. - - It is typically much faster to use the :attr:`ParameterVectorElement.index` property.""" - return self._params.index(value) - - def __getitem__(self, key): - return self.params[key] - - def __iter__(self): - return iter(self.params) - - def __len__(self): - return len(self._params) - - def __str__(self): - return f"{self.name}, {[str(item) for item in self.params]}" - - def __repr__(self): - return f"{self.__class__.__name__}(name={self.name!r}, length={len(self)})" - - def __getnewargs__(self): - return (self._name, len(self._params)) - - def __getstate__(self): - params = [p.__getstate__() for p in self._params] - return (self._name, params, self._root_uuid) - - def __setstate__(self, state): - self._name, params, self._root_uuid = state - self._params = [ParameterVectorElement(*p) for p in params] - - def resize(self, length): - """Resize the parameter vector. If necessary, new elements are generated. - - Note that the UUID of each :class:`.Parameter` element will be generated - deterministically given the root UUID of the ``ParameterVector`` and the index - of the element. In particular, if a ``ParameterVector`` is resized to - be smaller and then later resized to be larger, the UUID of the later - generated element at a given index will be the same as the UUID of the - previous element at that index. - This is to ensure that the parameter instances do not change. - - >>> from qiskit.circuit import ParameterVector - >>> pv = ParameterVector("theta", 20) - >>> elt_19 = pv[19] - >>> pv.resize(10) - >>> pv.resize(20) - >>> pv[19] == elt_19 - True - """ - if length > len(self._params): - root_uuid_int = self._root_uuid.int - self._params.extend( - [ - ParameterVectorElement(self, i, UUID(int=root_uuid_int + i)) - for i in range(len(self._params), length) - ] - ) - else: - del self._params[length:] +# Stubs because people keep insisting on importing from very non-public modules. +from qiskit._accelerate.circuit import ParameterVectorElement, ParameterVector diff --git a/qiskit/qpy/binary_io/value.py b/qiskit/qpy/binary_io/value.py index 5036c87d16a2..989ea880bc32 100644 --- a/qiskit/qpy/binary_io/value.py +++ b/qiskit/qpy/binary_io/value.py @@ -56,7 +56,7 @@ def _write_parameter(file_obj, obj): def _write_parameter_vec(file_obj, obj): - name_bytes = obj.vector._name.encode(common.ENCODE) + name_bytes = obj.vector.name.encode(common.ENCODE) file_obj.write( struct.pack( formats.PARAMETER_VECTOR_ELEMENT_PACK, diff --git a/releasenotes/notes/rust-parameter-vector-3fbac07411c36650.yaml b/releasenotes/notes/rust-parameter-vector-3fbac07411c36650.yaml new file mode 100644 index 000000000000..5eb442a8cd1c --- /dev/null +++ b/releasenotes/notes/rust-parameter-vector-3fbac07411c36650.yaml @@ -0,0 +1,4 @@ +--- +performance: + - :class:`.ParameterVector` was re-implemented in Rust. In is now approximately five times faster + to construct, and its vector elements require significantly less memory. diff --git a/test/python/circuit/test_parameters.py b/test/python/circuit/test_parameters.py index e693bf05ffb4..970bb40d0313 100644 --- a/test/python/circuit/test_parameters.py +++ b/test/python/circuit/test_parameters.py @@ -960,6 +960,13 @@ def test_parameter_vector_elements_can_be_recreated(self): other = ParameterVector("a", 5) self.assertNotEqual(list(left), list(other)) + def test_parameter_vector_backrefs_equal(self): + vector = ParameterVector("a", 10) + # With `ParameterVector` implemented in Rust and the actual Python object not cached, + # there's no guarantee that `pv[0].vector is pv` (as the pure-Python implementation used to + # preserve), but we need to maintain `pv[0].vector == pv`. + self.assertEqual([x.vector for x in vector], [vector] * len(vector)) + def test_binding_parameterized_circuits_built_in_multiproc(self): """Verify subcircuits built in a subprocess can still be bound.""" # ref: https://github.com/Qiskit/qiskit-terra/issues/2429