Skip to content

Python-free path for circuit parameters #14799

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 69 commits into from
Aug 6, 2025

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Jul 30, 2025

Summary

Follow up to #14757: use the Rust ParameterExpression in Param::ParameterExpression and expose a Python-free path for inserting, querying and assigning circuit parameters. The main goal of this is to unblock the Parameter-dependent transpiler passes, such as BasisTranslator, for the C API.

Details and comments

Open tasks:

  • Make Param::ParameterExpression Py-free
  • Provide a Py-free path for parameter assignment
  • Check if the vector logic in ParameterTable can be simplified

Cryoris added 30 commits June 5, 2025 13:54
-- 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
raynelfss
raynelfss previously approved these changes Aug 5, 2025
Copy link
Contributor

@raynelfss raynelfss left a comment

Choose a reason for hiding this comment

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

I like how this looks at the moment. I will await for @kevinhartman's final review. But this looks good to me :)

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

Looking pretty good. Most of my comments are minor.

circuit.h(4);
circuit.cp(PI2, 3, 4);
circuit.h(4);
circuit.h(4)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels weird to me that this kind of thing can fail. Though I suppose that's partly why we keep this experimental circuit builder stuff private to mcx.rs for now.

Long term, I would expect that any instruction-specific append methods on a CircuitData would not raise typing-related errors since their signatures would be specific to the instruction in question.

Copy link
Member

Choose a reason for hiding this comment

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

TBH, I was planning to rip all of this out after I saw it. We're wasting a bunch of cycles on repeated hash lookups so that the code looks familiar to the Python api. Realistically this should get a handful of interner indexes up front and just build PackedInstructions directly and call CircuitData::push. I was going to do a pass through all these examples to do that.

That being said here the fallibility is because the signature for the push_standard_gate() method takes a Param for the parameters. Since we now can express ParameterExpressions in rust we need to handle the case where the user passes those in. If the value contains any Param::ParameterExpressions it returns a PyErr if there is a name conflict on the parameter. That can't be triggered by this code though because it's all using Param::Float.

Copy link
Member

Choose a reason for hiding this comment

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

Joining the discussion late. The CircuitDataForSynthesis trait in mcx.rs has always been meant to be a temporary interface until we agree on something better, but which allows to easily port synthesis algorithms from Python to Rust as well as to implement new synthesis algorithms (such as the one in #14666).

Since some of the synthesis algorithms are quite complex (#14666 is a great example), I really want the notation to be ergonomic and clear more than anything; the main intention behind circuit.h method being that adding an H-gate to a circuit can be written in 1 line of code (which just happens to coincide with our old Python notation).

I agree that for simpler algorithms one should probably precompute interned indices upfront, but for more complex algorithms (#14666), the indices might be all over the place. And personally the code such as

circuit.cx(1, 2)
circuit.cx(2, 3)
circuit.cx(1, 2)
circuit.cx(2, 4)

seems much more clear to me than

q12 = interned_index(...)
q23 = interned_index(...)
q24 = interned_index(...)
circuit.push(cx, q12)
circuit.push(cx, q23)
circuit.push(cx, q12)
circuit.push(cx, q24)

(especially when some of the indices are ancillas[0] rather than just 1).

I am definitely open to suggestions.

The error condition isn't triggerable from the C API so instead of
having a message saying the impossible input is not valid, just calling
unwrap() is less verbose.
This commit updates the Param::ParameterExpression variant to store an
`Arc<ParameterExpression>` instead of `Box<ParameterExpression>`.
Internally the expression is all shared references with reference counting
and this also more closely models behavior from when it was previously a
PyObject (where `.clone()` was a ref count bump and not a full copy).
The impl of IntoPyObject was previously only implemented for a borrowed
ParameterExpression, which was just hiding a clone inside the impl. It's
better to be explicit about this and require the user clone manually so
they're aware of it. This commit switches the impl to be on
the owned ParameterExpression and updates the usage to explicitly clone.
This is just duplicated from the IntoPyObject impl.
@raynelfss raynelfss requested a review from kevinhartman August 6, 2025 13:18
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Aug 6, 2025
This commit expands the QkTarget API to support taking parameterized
gates that accept any angle value. Previously, this was not possible
because representing these gates internally in the target depended on
Python. That restriction has been removed with Qiskit#14799 and we can now
expose this functionality to C. This is a necessary component for being
able to build targets for current devices since they all typically
expose at least one gate that takes an arbitrary angle.

Fixes Qiskit#14704
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

@kevinhartman kevinhartman added this pull request to the merge queue Aug 6, 2025
Merged via the queue into Qiskit:main with commit 8fbd1b5 Aug 6, 2025
26 checks passed
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Aug 6, 2025
This commit expands the QkTarget API to support taking parameterized
gates that accept any angle value. Previously, this was not possible
because representing these gates internally in the target depended on
Python. That restriction has been removed with Qiskit#14799 and we can now
expose this functionality to C. This is a necessary component for being
able to build targets for current devices since they all typically
expose at least one gate that takes an arbitrary angle.

Fixes Qiskit#14704
alexanderivrii added a commit to alexanderivrii/qiskit-terra that referenced this pull request Aug 7, 2025
github-merge-queue bot pushed a commit that referenced this pull request Aug 7, 2025
* Adding the MCX synthesis code based on PLDI24 paper

Co-authored-by: Jens Palsberg <palsberg@ucla.edu>
Co-authored-by: Keli Huang <kelihuang@cs.ucla.edu>

* Adding new funcion to init and docs

* adding test (for small values for the number of controls)

* adding plugin + test for the new method

* modifying the default MCX plugin to use the new method when appropriate

* adding release notes

* doctrings

* fixing comment about v24; adding more tests; improving bounds

* adding unitary simulator in Rust

Consructs a unitary matrix from a CircuitData with only unitary operations, exactly as
what Operator(quantum_circuit).data does.

The code is a simple wrapper on top of unitary_compose.

* adding simple test

* ignoring barriers during simulation

* change error to QiskitError

* splitting code into Rust and Python facing

* adding correctness test for increment_n_dirty

* improving release note

* improving documentation

* more docs

* updating the code following the changes in #14799

* disabling miri on the rust tests: it's way too slow

* updating release notes following review

---------

Co-authored-by: Jens Palsberg <palsberg@ucla.edu>
Co-authored-by: Keli Huang <kelihuang@cs.ucla.edu>
github-merge-queue bot pushed a commit that referenced this pull request Aug 7, 2025
* Add support for parameterized gates to the Target C API

This commit expands the QkTarget API to support taking parameterized
gates that accept any angle value. Previously, this was not possible
because representing these gates internally in the target depended on
Python. That restriction has been removed with #14799 and we can now
expose this functionality to C. This is a necessary component for being
able to build targets for current devices since they all typically
expose at least one gate that takes an arbitrary angle.

Fixes #14704

* Apply suggestions from code review

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

---------

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
raynelfss pushed a commit to raynelfss/qiskit that referenced this pull request Aug 11, 2025
* Split py and py-free methods

* playing with Symbol as Parameter

-- 606 failing tests

* fix errors

* about to start the mess

* suite passing

* PVE in Rust

* start assign

* add assign

* dealing with unknown params and conflicts

* an attempt at vector fixing

* fix more tests

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

* implement name_map as track record

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

- 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

* implement _qpy_replay

* implement sympify

* fix copy/deepcopy

* Implement pickle via QPY and Parameter.assign to return Parameter

* fix template matching

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

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

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

* clippy

* a lot of docs

* rename PyParameterExpression to ParameterExpression

* Thou shan't fake hyperrefs

* start to pull the thread on Param::ParameterExpression

* proper split: (Py)ParameterExpression

* rm pyclass from Symbol

* TODO's and no more RwLock

- rm Arc<RwLock<..>> for inner, since expressions are immutable and not shared, this is not needed
- address a bunch of todos, that include
  - rm rsub/rdiv/rpow
  - introduce try_to_value and _to_symbol
  - make internal data private

* First round of review comments

mainly removing some unnecessary clones, and fixing Hash for ParameterExpression

* rm `name_map` from PyParameterExpression

--- and just reconstruct the Py symbols from ParameterExpression.name_map

* Fix vector element parsing

Co-authored-by: "U-AzureAD\JUNDOI" doichan@jp.ibm.com

* more thread pulling

made the ParameterTable py-free

* finish 1st review round

mainly rm unused code and dangling comments

* fix symbol iteration

we want to iterate over name_map to capture 0*x, not the symbolic tree

* some problems with float/Param types left

* type fixes

A bunch of precautions around preserving integers and validating types, especially around Delay. One problem was that we used Param::Obj to represent bound, complex numbers, but this is now represented by Param::ParameterExpression. That means we need an additional "is numeric" check in the custom gate case, which we could've avoided before.

* resolve some todos, less py-paths

one thing we could investigate is if we can simplify the parameter table's logic around vector elements

* playing rm vector

* rm vector

* housekeeping

a bunch of renaming and removing py-specific comments

* add `CircuitData::parameters`

* Low hanging fruits

Mostly fixing up redundant allocations and preferring refs over values where possible

* Fix clippy failures

* Use Arc<Symbol> for SymbolExpr::Symbol

The size of Symbol is 56 bytes and when put in an enum for SymbolExpr
this brings the size of the enum up to 56 bytes because it's the largest
element. Since SymbolExpr ends up going in Param this brings Param up to
96 bytes from 16 prior to this PR which is a huge jump in the memory
footprint for somethign we put on every instruction in the circuit
whether there is a parameter or not. This commit boxes the symbol in
SymbolExpr to reduce the footprint of the enum down to 32 bytes because
of the Value field which is 24 bytes and the rest is because of 8 byte
alignment and the discriminant. This brings the Param enum down to 72
bytes which is still huge compared to the original 16 but is an
improvement. The next step here is going to be to box
`ParameterExpression` in `Param::ParameterExpression` which will reduce
it to 8 bytes in the Param enum and shrink the footprint back to it's
original size. In a follow up PR we should investigate optimizing the
memory footprint of SymbolExpr even more because the overhead is quite
high right now.

* Use Box<ParameterExpression> for Param::ParameterExpression

The Param::ParameterExpression object contains a ParameterExpression
object which is quite large compared to when it was just a PyObject.
When it was a PyObject it took 8 bytes because a PyObject is just a
pointer to the Python heap. However, the new ParameterExpression object
is significantly larger at 72 bytes, 32 bytes for the SymbolExpr (after
the previous commit) and 40 bytes for the HashMap. Putting this directly
in the enum makes each instance of Param 72 bytes even if there is no
parameter expressions in the circuit. This balloons the memory usage of
Qiskit everywhere because we've just increased the memory footprint of
anything with parameters 4.5x (Param was 16 bytes before because of
the discriminant and alignment). While we should work to reduce the memory
footprint and improve the efficiency of the `ParameterExpression` type
as much as we can, it's unlikely we'll ever get a `ParameterExpression`
down to 8 bytes. This commit updates Param::ParameterExpression to
contain a Box<ParameterExpression> instead so that we're only storing a
pointer to a heap allocated ParameterExpression object which reduces the
size of Param back to what it was prior to this PR branch.

* Move int and complex back to Param::Obj

The Param enum was intended to provide a quick low overhead check to
filter based on whether an instruction has real values, an expression,
or something custom defined for Python gates. In the Python data model
literally any object could be a "parameter" for a gate, as long as a
subclass overrides `.validate_parameter()` to say something is allowed.
The rust data model is more explicit in that everything is either a
float or a parameter expression in Rust, and anything outside of that is
necessarily something custom for a Python object (int is the exception
as it should be allowed in the data model but historically wasn't
because nothing ported to rust prior to Delay needed it, this will need
to be fixed). This is true for numeric types that ParameterExpression
can work with, mainly integer and complex numbers. If they are present
as a param that is only valid for an object coming from Python and
aren't part of the core data model. This enables things consuming
circuits in Rust to be able to filter out contextually on whether
something is parameterized or not.

Previously this PR branch moved these numeric types to be in the
Param::ParameterExpression where they can be stored in their rust
native data type (although arguably int was lossy and could cause
issues for values outside what's expressible in `i64`). However this
broke the above assumptions which added a special condition on anything
consuming parameters in Rust, where `Param::ParameterExpression` became
the indicator of a parameter expression or parameter that was unbound
or a numeric int or complex number in custom contexts from Python. This
was a potential source of bugs as any user of the Param interface would
need to know that they might have to check ParameterExpression manually
even if their operations didn't support unbound expression. Mostly this
would likely come up in transpiler passes that often have short cuts
like:

```rust
if matches!(param, Param::ParameterExpression(_)) {
    continue;
}
```

which might not be correct if there is a bound numeric value in the
expression. This commit switches the logic back to using `Obj` to
indicate a custom Python parameter object. Even if it is a numeric
value that could be represented by a bound `ParameterExpression`

A follow up here though is we should actually define int in `Param`
so we can correctly model `dt` for Delay in Rust space. As this is part
of our standard data model, even if it's only for a single type.

* Clarify ParameterTable comment

* Tune PyParameterExpression::extract_coerce()

This commit updates the extract_coerce() method of the
PyParameterExpression struct. During binding (called from Python) this
method gets called for every value and param in the bind input so the
runtime performance is very important for binding performance. Since
the method relies on basically python type checking with python
pseudocode logic like:

```python
if isinstance(x, int):
    return_expr = Value(int)
elif isinstance(x, float):
    return_expr = Value(float)
elif isinstance(x, complex):
    return_expr = Value(complex)
elif isinstance(x, ParameterVector):
    return_expr = Symbol(vector_element)
elif isinstance(x, Parameter)
    return_expr = Symbol(param)
else:
    return_expr = x
```

we need each successive check to be as fast as possible for the overall
function performance. The function previously was using
`Bound::extract()` to do the type checking here, but this is
unecessarily heavy weight especially in the negative case. This is
because it builds a real PyErr to return to the user, while we don't
actually ever use the Err case so it's safe to ignore it. This was
actually visible in profiles, especially for Complex which building the
result object was particularly prominent. The `.downcast()` method lets
us do a faster validation of the Python type before converting it to a
rust object avoiding this overhead. This is actually called out in the
performance guidance in the PyO3 user guide:

https://pyo3.rs/v0.25.1/performance.html#extract-versus-downcast

* Add parameter tracking to CircuitData convenience push methods

CircuitData had two methods `push_standard_gate()` and
`push_packed_operation()` which are convenience methods for appending
gates to a circuit from rust. Prior to this PR those only would have
ever received `Param::Float()` for their parameter input in a rust
context because Parameters were the domain of Python. As a small
optimization those methods skipped calling the parameter tracking
routine because it was essentially a no-op and unnecessary overhead
because there was never the possibility of anything needing to be
tracked. However, this has now changed with this PR and those
functions need to be tracking any Parameters they receive before adding
an operation to the circuit. This commit fixes this oversight and
changes the `self.data.push()` used internally to append to the
instruction vec to `self.push()` which does the same thing but also
has parameter tracking.

This has the knock-on effect of causing a PyResult<()> to be returned
by these methods because the tracking currently returns a PyResult. In
the C API in particular these are ignored because there is no context
from the existing C API where parameters can exist so the tracking call
is a no-op and won't ever return an Err.

* Switch from .expect() to .unwrap() in cext/circuit.rs

The error condition isn't triggerable from the C API so instead of
having a message saying the impossible input is not valid, just calling
unwrap() is less verbose.

* Fix typo symbolj -> symbol

* Update comment about the Python path in parameter binding for clarity

* Switch Param::ParameterExpression from Box<_> to Arc<_>

This commit updates the Param::ParameterExpression variant to store an
`Arc<ParameterExpression>` instead of `Box<ParameterExpression>`.
Internally the expression is all shared references with reference counting
and this also more closely models behavior from when it was previously a
PyObject (where `.clone()` was a ref count bump and not a full copy).

* Only implement IntoPyObject for owned ParameterExpression

The impl of IntoPyObject was previously only implemented for a borrowed
ParameterExpression, which was just hiding a clone inside the impl. It's
better to be explicit about this and require the user clone manually so
they're aware of it. This commit switches the impl to be on
the owned ParameterExpression and updates the usage to explicitly clone.

* Standardize on symbol naming convention for ParameterTable api

* Remove duplicate Symbol::coerce_into_py() method

This is just duplicated from the IntoPyObject impl.

* Remove stale comment

* Fix TODO comment in qasm3 experimental exporter

* Clarify comment on custom IntoPyObject impl

* Add TODO comment about not using Box<dyn Iterator>

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library mod: transpiler Issues and PRs related to Transpiler performance priority: high Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Oxidize ParameterExpression
7 participants