[Alternative] Move Parameter to Rust #14757

Cryoris · 2025-07-18T09:59:58Z

Summary

Based on the excellent work in #14207, this is PR implements a slightly alternative approach to porting the ParameterExpression to Rust, based on discussions with @doichanj, @kevinhartman and @mtreinish. The main differences are outlined below. We should compare both approaches and check which one is closer to what we'd like. Of course the efforts could also be merged.

Details and comments

This PR does not supersede #14207, it is merely a slightly different approach and it's majority is based on #14207. The main differences are:

Keep ParameterExpression, Parameter and ParameterVectorElement fully in Rust, without a Python-side class. This is done to reduce the code base and maintainer effort. This could also improve performance as we need less back and forth between the Rust and Python boundaries.
Store the symbol's UUID on the Symbol in the symbolic expression engine itself, rather than keep a separate map of symbols to their UUID. This is done to couple the UUID and symbol closer instead of relying on a third object to make this connection.
Implement pickling via QPY instead of string serialization -- we already have a serialization format which we could use. Maybe this allows to remove string-based operations in the future for safety?
... some other bits I probably forgot

Things that could make this PR better (maybe in this PR or a follow up)

Keep a separate ParameterExpression and PyParameterExpression for clearer Python split. Doing this would allow us to make the expression's name_map use Symbols directly. Let's think about how we want to use this from C!

Make Symbol not a pyclass (probably easy to do, just didn't get to it yet)
Should we remove the index from Symbol and have an IndexedSymbol, like in Move Parameter classes from Python to Rust #14207?
try to remove string parsing
... some other bits I surely did forget and am unaware of

Co-authored-by: "U-AzureAD\JUNDOI" [email protected]

-- 606 failing tests

- name conflicts - hash(2.3) == hash(ParamExpr(2.3)) - some error messages

This allows keeping track of which parameters are still in the expression, even though the expression is optimized. E.g. x*0 = 0, but x is still valid to bind.

- don't use UUID in to_string, only in string_id (using strings for comparisons should probably be avoided in future) - clippy - relax Python is-checks to eq-checks

this is rather a temporary solution, maybe we ought to disable string parsing in favor or a proper SymPy->parameter expression conversion

qiskit-bot · 2025-07-18T10:00:05Z

One or more of the following people are relevant to this code:

@Cryoris
@Qiskit/terra-core
@ajavadia
@mtreinish
@nkanazawa1989

coveralls · 2025-07-18T11:39:14Z

Pull Request Test Coverage Report for Build 16525568695

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

1338 of 1648 (81.19%) changed or added relevant lines in 13 files are covered.
202 unchanged lines in 11 files lost coverage.
Overall coverage decreased (-0.2%) to 87.602%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/parameter/symbol_parser.rs	1	2	50.0%
qiskit/circuit/rust_parameter_expression.py	0	4	0.0%
qiskit/qpy/binary_io/value.py	18	26	69.23%
crates/circuit/src/parameter_table.rs	3	15	20.0%
crates/circuit/src/parameter/symbol_expr.rs	225	301	74.75%
crates/circuit/src/parameter/parameter_expression.rs	1025	1234	83.06%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.53%
crates/transpiler/src/passes/elide_permutations.rs	3	94.92%
crates/circuit/src/parameter_table.rs	4	87.85%
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py	4	94.62%
crates/quantum_info/src/convert_2q_block_matrix.rs	7	92.59%
crates/transpiler/src/passes/gate_direction.rs	11	96.05%
crates/transpiler/src/passes/consolidate_blocks.rs	12	94.12%
crates/qasm2/src/parse.rs	18	96.62%
qiskit/qpy/common.py	21	55.24%
crates/transpiler/src/target/mod.rs	48	84.99%

Totals
Change from base Build 16327707788:	-0.2%
Covered Lines:	81812
Relevant Lines:	93391

💛 - Coveralls

doichanj · 2025-07-24T03:47:09Z

crates/circuit/src/parameter/symbol_parser.rs

@@ -74,9 +76,9 @@ fn parse_symbol(s: &str) -> IResult<&str, SymbolExpr, VerboseError<&str>> {
                    // if array indexing is required in the future
                    // add indexing in Symbol struct
                    let s = format!("{v}[{i}]");
-                    Ok(SymbolExpr::Symbol(Arc::new(s)))
+                    Ok(SymbolExpr::Symbol(Symbol::new(&s, None, None)))


Here we parse index of vector element, so Symbol can be initialized as

Suggested change

Ok(SymbolExpr::Symbol(Symbol::new(&s, None, None)))

Ok(SymbolExpr::Symbol(Symbol::new(v, None, Some(i))))

kevinhartman

Adding just a couple of comments before you push the PyParameterExpression split stuff.

kevinhartman · 2025-07-18T14:19:10Z

crates/circuit/src/parameter/parameter_expression.rs

+    UnknownParameter(Symbol),
+    #[error("Cannot bind following parameters not present in expression: {0:?}")]
+    UnknownParameters(HashSet<Symbol>),


These should be combined I'd think, since otherwise callers have to handle both errors.

kevinhartman · 2025-07-21T18:19:02Z

crates/circuit/src/circuit_data.rs

+            // let as_str = expr.call_method0("__str__")?;
+            // let params = expr.getattr(parameters_attr)?;
+            // println!("expr {as_str:?} {params:?}");


Suggested change

// let as_str = expr.call_method0("__str__")?;

// let params = expr.getattr(parameters_attr)?;

// println!("expr {as_str:?} {params:?}");

kevinhartman · 2025-07-21T18:19:18Z

crates/circuit/src/circuit_data.rs

+            // let as_str = new_expr.call_method0("__str__")?;
+            // let params = new_expr.getattr(parameters_attr)?;
+            // println!("new_expr {as_str:?} {params:?}");


Suggested change

// let as_str = new_expr.call_method0("__str__")?;

// let params = new_expr.getattr(parameters_attr)?;

// println!("new_expr {as_str:?} {params:?}");

kevinhartman · 2025-07-21T18:22:18Z

crates/circuit/src/parameter/parameter_expression.rs

+    }
+}
+
+/// Attempt to extract a [PyParameterExpression] from a bound [PyAny].


Suggested change

/// Attempt to extract a [PyParameterExpression] from a bound [PyAny].

/// Attempt to extract a [ParameterExpression] from a bound [PyAny].

kevinhartman · 2025-07-21T18:25:02Z

crates/circuit/src/parameter/parameter_expression.rs

+    // A map keeping track of all symbols, with their name. This map *must* be kept
+    // up to date upon any operation performed on the expression.
+    // TODO it would be nicer to just store [Symbol]s as values, which can maybe be done
+    // with a secondary PyParameterExpression storing the [PyParameter] in a map.


This comment seems to imply that ParameterExpressions are mutable. I guess they are technically because these fields are public, but would it be better if they weren't? It looks like in most cases any operation on a ParameterExpression results in a brand new instance.

They are immutable -- this was a comment targeting devs 🙂 With only the double backslash this shouldn't show up in any docs, right?

It's a good idea to make these attributes private. I think some attributes I just marked public because I needed them at some points where they weren't accessible. I'll go through to check that they are correctly private 👍🏻

kevinhartman · 2025-07-21T18:30:57Z

crates/circuit/src/parameter/parameter_expression.rs

+        }
+    }
+
+    /// Substitute symbols with [PyParameterExpression]s.


Suggested change

/// Substitute symbols with [PyParameterExpression]s.

/// Substitute symbols with [ParameterExpression]s.

Probably should do a replace-all for this.

kevinhartman

Not a complete review yet, but submitting what I have so far. I'll plan to finish up on Monday 🙂.

kevinhartman · 2025-07-25T19:00:44Z

crates/circuit/src/parameter/parameter_expression.rs

+    // A map keeping track of all symbols, with their name. This map *must* be kept
+    // up to date upon any operation performed on the expression.


The reason this comment bugs me is that it makes it seem like as a developer, I should expect to mutate a ParameterExpression while performing operations that involve it. But it sounds like from your earlier reply that this would only ever be set once during construction. Maybe tweak it to say "This map must have entries for all symbols used by the expression."

kevinhartman · 2025-07-25T19:02:33Z

crates/circuit/src/parameter/parameter_expression.rs

+
+impl Hash for ParameterExpression {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.expr.to_string().hash(state);


Just to be sure, it's not possible for two different expressions from a tree-perspective to have the same string representation, correct?

kevinhartman · 2025-07-25T19:13:40Z

crates/circuit/src/parameter/parameter_expression.rs

+                match self.expr.eval(true) {
+                    Some(e) => e.to_string(),
+                    None => self.expr.optimize().to_string(),


Why should we evaluate or optimize the expression? Isn't it better to just display it exactly as it is? Imagine if we compare two expressions for equality and find that they aren't equal, but then when we print, they evaluate to the same thing.

kevinhartman · 2025-07-25T19:19:10Z

crates/circuit/src/parameter/parameter_expression.rs

+    ///
+    /// Caution: The caller **guarantees** that ``name_map`` is consistent with ``expr``.
+    /// If uncertain, call [Self::from_symbol_expr], which automatically builds the correct name map.
+    pub fn new(expr: &SymbolExpr, name_map: &HashMap<String, Symbol>) -> Self {


What is the use-case for creating a custom mapping, rather than reading it from the SymbolExpr?

In any case, this function should at least take both of its inputs by value and force the caller to do the clone. This is better practice in general since it is honest about the ownership needs of the call.

kevinhartman · 2025-07-25T19:19:37Z

crates/circuit/src/parameter/parameter_expression.rs

+    }
+
+    /// Construct from a [Symbol].
+    pub fn from_symbol(symbol: &Symbol) -> Self {


Suggested change

pub fn from_symbol(symbol: &Symbol) -> Self {

pub fn from_symbol(symbol: Symbol) -> Self {

Same comment

kevinhartman · 2025-07-25T20:11:55Z

crates/circuit/src/parameter/parameter_expression.rs

+///
+/// This is backed by Qiskit's symbolic expression engine and a cache
+/// for the parameters inside the expression.
+#[pyclass(


I think you can just add hash to this list if you want to skip having __hash__.

kevinhartman · 2025-07-25T20:12:39Z

crates/circuit/src/parameter/parameter_expression.rs

+pub struct PyParameterExpression {
+    pub inner: Arc<RwLock<ParameterExpression>>,
+    // in contrast to ParameterExpression::name_map, this stores [PyParameter]s as value
+    pub name_map: HashMap<String, PyParameter>,


We already talked about this offline, but it feels like it's probably better not to have this mapping if we already have one inside ParameterExpression.

kevinhartman · 2025-07-25T20:14:35Z

crates/circuit/src/parameter/parameter_expression.rs

+        let name_map = value
+            .name_map
+            .iter()
+            .map(|(name, symbol)| (name.clone(), PyParameter::new(symbol)))
+            .collect();


Suggested change

let name_map = value

.name_map

.iter()

.map(|(name, symbol)| (name.clone(), PyParameter::new(symbol)))

.collect();

let name_map = value

.name_map

.into_iter()

.map(|(name, symbol)| (name, PyParameter::new(symbol)))

.collect();

maybe?

kevinhartman · 2025-07-25T20:15:48Z

crates/circuit/src/parameter/parameter_expression.rs

+    fn extract_coerce(ob: &Bound<'_, PyAny>) -> PyResult<Self> {
+        if let Ok(i) = ob.extract::<i64>() {
+            Ok(
+                ParameterExpression::new(&SymbolExpr::Value(Value::from(i)), &HashMap::new())


Yes, this callsite definitely looks like it wants to take its arguments by value!

kevinhartman · 2025-07-25T22:43:15Z

crates/circuit/src/parameter/parameter_expression.rs

+
+    /// Check if the expression corresponds to a plain symbol.
+    ///
+    /// TODO can we delete this? Not part of public interface before.


If we don't, then it will be. Just making a note here to delete or prefix with _.

Cryoris added 25 commits June 5, 2025 13:54

Split py and py-free methods

06f1f5d

playing with Symbol as Parameter

fb35a74

-- 606 failing tests

fix errors

f52ca71

about to start the mess

b4b2635

suite passing

a3fa027

PVE in Rust

6ac10c9

start assign

8f495fd

add assign

2ab1a97

dealing with unknown params and conflicts

c79f36e

an attempt at vector fixing

803a777

fix more tests

103dbd9

- name conflicts - hash(2.3) == hash(ParamExpr(2.3)) - some error messages

implement name_map as track record

3df71fd

This allows keeping track of which parameters are still in the expression, even though the expression is optimized. E.g. x*0 = 0, but x is still valid to bind.

The string debacle

2b561dc

- don't use UUID in to_string, only in string_id (using strings for comparisons should probably be avoided in future) - clippy - relax Python is-checks to eq-checks

some housekeeping

b6b21af

implement _qpy_replay

04c1030

implement sympify

2aec3de

fix copy/deepcopy

775a379

Implement pickle via QPY and Parameter.assign to return Parameter

19f23d2

fix template matching

72e8eeb

this is rather a temporary solution, maybe we ought to disable string parsing in favor or a proper SymPy->parameter expression conversion

fix rhs - lhs mixup

e53442f

fix more tests on binding {x: 0*y}

ce6e863

fix last failing tests (see numeric(strict=False))

5ae9d5a

clippy

58eb92b

a lot of docs

dfbdbe9

rename PyParameterExpression to ParameterExpression

3197c5b

Cryoris requested a review from a team as a code owner July 18, 2025 09:59

Cryoris added the Rust This PR or issue is related to Rust code in the repository label Jul 18, 2025

Merge branch 'main' into rust-param-pyparam

9a07ded

Thou shan't fake hyperrefs

f2a3af0

doichanj reviewed Jul 24, 2025

View reviewed changes

proper split: (Py)ParameterExpression

28622b6

kevinhartman reviewed Jul 24, 2025

View reviewed changes

rm pyclass from Symbol

d6ebb58

kevinhartman reviewed Jul 25, 2025

View reviewed changes

	Ok(SymbolExpr::Symbol(Symbol::new(&s, None, None)))
	Ok(SymbolExpr::Symbol(Symbol::new(v, None, Some(i))))

	// let as_str = expr.call_method0("__str__")?;
	// let params = expr.getattr(parameters_attr)?;
	// println!("expr {as_str:?} {params:?}");

	// let as_str = new_expr.call_method0("__str__")?;
	// let params = new_expr.getattr(parameters_attr)?;
	// println!("new_expr {as_str:?} {params:?}");

	/// Attempt to extract a [PyParameterExpression] from a bound [PyAny].
	/// Attempt to extract a [ParameterExpression] from a bound [PyAny].

	/// Substitute symbols with [PyParameterExpression]s.
	/// Substitute symbols with [ParameterExpression]s.

		// A map keeping track of all symbols, with their name. This map must be kept
		// up to date upon any operation performed on the expression.

	pub fn from_symbol(symbol: &Symbol) -> Self {
	pub fn from_symbol(symbol: Symbol) -> Self {

[Alternative] Move Parameter to Rust #14757

Are you sure you want to change the base?

[Alternative] Move Parameter to Rust #14757

Uh oh!

Conversation

Cryoris commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented Jul 18, 2025

Uh oh!

coveralls commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16525568695

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

doichanj Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinhartman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinhartman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Cryoris commented Jul 18, 2025 •

edited

Loading

coveralls commented Jul 18, 2025 •

edited

Loading

doichanj Jul 24, 2025 •

edited

Loading