Skip to content

Add ParameterExpression in Rust #13278

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 96 commits into from
May 16, 2025

Conversation

doichanj
Copy link
Collaborator

@doichanj doichanj commented Oct 4, 2024

Summary

This PR is a new implementation of ParameterExpression written in Rust for issue #13267

Details and comments

This PR implements our own symbolic expression library written in Rust, this replaces symengine from ParameterExpression class.

crates/circuit/symbol_expr.rs is a symbolic expression module. Equations with symbols and values are expressed by using AST structure with nodes of operations. This module contains all the operators defined in Python's ParameterExpression class.

To separate Rust's implementation from Python we prepared Python interface class crates/circuit/symbol_expr_py.rs that is called from ParameterExpression in Python, instead of symengine.

crates/circuit/parameterexpression.rs is the ParameterExpression class for Rust. It provides similar functionalities to Python version. (but maybe we have to add more)

@doichanj doichanj requested a review from a team as a code owner October 4, 2024 10:07
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Nov 8, 2024

Pull Request Test Coverage Report for Build 15065615407

Details

  • 2151 of 2811 (76.52%) changed or added relevant lines in 9 files are covered.
  • 16 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.4%) to 88.302%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/qpy/binary_io/value.py 0 4 0.0%
crates/circuit/src/symbol_parser.rs 165 193 85.49%
crates/circuit/src/parameter_expression.rs 278 368 75.54%
crates/circuit/src/symbol_expr.rs 1655 2193 75.47%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 1 91.98%
crates/transpiler/src/passes/unitary_synthesis.rs 1 94.85%
qiskit/circuit/parameterexpression.py 2 96.97%
crates/qasm2/src/parse.rs 12 96.68%
Totals Coverage Status
Change from base Build 15055964365: -0.4%
Covered Lines: 78448
Relevant Lines: 88841

💛 - Coveralls

@mtreinish
Copy link
Member

Add complex number support (is it necessary ?)

@Cryoris can confirm, but I think on main we don't have to worry about this for 2.0 since we're dropping pulse support. AFAIK the complex parameter binding and handling was only necessary for pulse expressions. But maybe there is a use case I'm overlooking.

@Cryoris
Copy link
Contributor

Cryoris commented Nov 12, 2024

For circuits, real is enough. But we currently also allow parameter expressions in the SparsePauliOp, where there's no restriction to real values. Beyond being more difficult to implement, complex number support doesn't come with a performance hit, does it?

@ShellyGarion ShellyGarion linked an issue Nov 27, 2024 that may be closed by this pull request
@1ucian0 1ucian0 added the Rust This PR or issue is related to Rust code in the repository label Dec 11, 2024
_ => false,
},
SymbolExpr::Unary(e) => e.expr.is_complex(),
SymbolExpr::Binary(e) => e.lhs.is_complex() || e.rhs.is_complex(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this get us into trouble in expressions like 1j * 1j, i.e. where LHS and RHS are complex but the expression is not? (Same question for is_real)

@ShellyGarion
Copy link
Member

I just wanted to point out that not all of the ParameterExpression methods are documented in the API docs.
One example is pow another one in mul (see #13510).
As part of this PR some other methods (like complex) are being added, so we need to make sure that everything is well documented and tested.

pub fn complex(&self) -> Option<Complex64> {
match self.eval(true) {
Some(v) => match v {
Value::Real(_) => Some(0.0.into()),
Copy link
Member

Choose a reason for hiding this comment

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

any real number is also a complex number. so if r is real, shouldn't complex(r)=r+0*i ?

}

pub fn rcp(self) -> SymbolExpr {
match self {
Copy link
Member

Choose a reason for hiding this comment

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

what is rcp? is it 1/x?

Value::Complex(e) => Value::Complex(e.ln()),
}
}
pub fn sqrt(&self) -> Value {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is also useful as a stand-alone fucntion

}
pub fn sqrt(&self) -> Value {
match self {
Value::Real(e) => Value::Real(e.sqrt()),
Copy link
Member

Choose a reason for hiding this comment

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

what if you give sqrt a real negative number? will you get an error or a complex result?

}
}

pub fn is_minus(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

maybe is_negative is a better name?

@doichanj
Copy link
Collaborator Author

I'm trying to fix errors in test cases but I'm feeling difficulty to solve complicated test cases (e.g. test.python.quantum_info.operators.symplectic.test_sparse_pauli_op.TestSparsePauliOpMethods.test_compose_7)
Is there good way to debug these test cases?

doichanj and others added 3 commits May 16, 2025 17:48
Co-authored-by: Julien Gacon <gaconju@gmail.com>
- Update docs: we no longer rely on symengine/sympy for parameters
- Reword reno: we still install symengine/sympy at this point
- Fix conjugate derivative: since conj() acts as identity on real numbers, the derivative is 1
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Thanks for all the work @doichanj! I played quite a bit with this and ran some benchmarks for using parameters in circuits, which all looked unchanged to before (so very good 🙂). With the additional tests for the parameter functions and derivatives I'm feeling quite comfortable merging this now.

I think we ought to put some note about this change in the prelude (or add an upgrade note at least) to point out that this internal change could introduce changes when dealing with parameters -- I expect we missed some edge cases that we changed unintentionally.

I'd hold off putting this in the merge queue for after a second approval (@kevinhartman or also someone else who is comfortable with this PR).

@kevinhartman kevinhartman added this pull request to the merge queue May 16, 2025
Merged via the queue into Qiskit:main with commit 1a2b8df May 16, 2025
24 checks passed
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 16, 2025
In Qiskit#13278 we removed our dependency on symengine and replaced it with
our own symbolic engine. However that PR neglected to remove symengine
from the requirements list. This commit corrects this oversight and
removes it from the requirements list. It also updates the release notes
to make them a bit more prescise and document the change in requirements
and potential behavior differences with the new engine.
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 16, 2025
In Qiskit#13278 we removed our dependency on symengine and replaced it with
our own symbolic engine. However that PR neglected to remove symengine
from the requirements list. This commit corrects this oversight and
removes it from the requirements list. It also updates the release notes
to make them a bit more prescise and document the change in requirements
and potential behavior differences with the new engine.
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 16, 2025
In the recently merged Qiskit#13278 the implementation for the sympify
method for ParameterExpression was changed because we no longer rely on
symengine internally. Previously we would just return the inner
symengine object used to represent the symbolic expression. Without
symengine available Qiskit#13278 updated the implementation of the method to
generate a string representation of the expression and pass that to
sympify which has a "parser" for converting that expression string to a
sympy object. However, sympify() method is insecure as it internally
relies on Python's eval() to parse the string and can't be used for
untrusted input. While this doesn't have the same exact exposure as
in GHSA-6m2c-76ff-6vrf
because you have to opt-in to using this function with input that is
untrusted and the degrees of freedom are less because it has to go
through the rust symbolic expression it is still a potential
vulnerability waiting to happen. This commit reworks the sympify
implementation to avoid using sympy's parser and instead just builds
the sympy expression from the internal state directly.
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 16, 2025
In the recently merged Qiskit#13278 the implementation for the sympify
method for ParameterExpression was changed because we no longer rely on
symengine internally. Previously we would just return the inner
symengine object used to represent the symbolic expression. Without
symengine available Qiskit#13278 updated the implementation of the method to
generate a string representation of the expression and pass that to
sympify which has a "parser" for converting that expression string to a
sympy object. However, sympify() method is insecure as it internally
relies on Python's eval() to parse the string and can't be used for
untrusted input. While this doesn't have the same exact exposure as
in GHSA-6m2c-76ff-6vrf
because you have to opt-in to using this function with input that is
untrusted and the degrees of freedom are less because it has to go
through the rust symbolic expression it is still a potential
vulnerability waiting to happen. This commit reworks the sympify
implementation to avoid using sympy's parser and instead just builds
the sympy expression from the internal state directly.
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 17, 2025
With the move to symbolic expression class added in Qiskit#13278 the
dependency on sympy is no longer as core to the Qiskit package as it was
in the past. The only pieces that are still reliant on it are QPY
deserialization of payloads older than QPY v13 and some small
visualization utilities where sympy is used to generate the output.
Since this functionality is not core to the library and only used in
some less common use cases we no longer need to install sympy for every
installation of Qiskit. In preparation for this Qiskit 2.0 laid the
groundwork by marking all uses of sympy in Qiskit as being fallible if
sympy was not installed and documented that sympy is not guaranteed to
be installed with Qiskit anymore.

This commit implements this change and moves sympy to be an optional
dependency only used by some small components.
github-merge-queue bot pushed a commit that referenced this pull request May 21, 2025
* Avoid using string parsing for ParameterExpression.sympify()

In the recently merged #13278 the implementation for the sympify
method for ParameterExpression was changed because we no longer rely on
symengine internally. Previously we would just return the inner
symengine object used to represent the symbolic expression. Without
symengine available #13278 updated the implementation of the method to
generate a string representation of the expression and pass that to
sympify which has a "parser" for converting that expression string to a
sympy object. However, sympify() method is insecure as it internally
relies on Python's eval() to parse the string and can't be used for
untrusted input. While this doesn't have the same exact exposure as
in GHSA-6m2c-76ff-6vrf
because you have to opt-in to using this function with input that is
untrusted and the degrees of freedom are less because it has to go
through the rust symbolic expression it is still a potential
vulnerability waiting to happen. This commit reworks the sympify
implementation to avoid using sympy's parser and instead just builds
the sympy expression from the internal state directly.

* Remove unused Rust functions that support sympy string generation

* Add test coverage for all of parameter expression

* Add .sign() to the megaexpression
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 21, 2025
With the move to symbolic expression class added in Qiskit#13278 the
dependency on sympy is no longer as core to the Qiskit package as it was
in the past. The only pieces that are still reliant on it are QPY
deserialization of payloads older than QPY v13 and some small
visualization utilities where sympy is used to generate the output.
Since this functionality is not core to the library and only used in
some less common use cases we no longer need to install sympy for every
installation of Qiskit. In preparation for this Qiskit 2.0 laid the
groundwork by marking all uses of sympy in Qiskit as being fallible if
sympy was not installed and documented that sympy is not guaranteed to
be installed with Qiskit anymore.

This commit implements this change and moves sympy to be an optional
dependency only used by some small components.
github-merge-queue bot pushed a commit that referenced this pull request May 22, 2025
* Remove symengine from the requirements list

In #13278 we removed our dependency on symengine and replaced it with
our own symbolic engine. However that PR neglected to remove symengine
from the requirements list. This commit corrects this oversight and
removes it from the requirements list. It also updates the release notes
to make them a bit more prescise and document the change in requirements
and potential behavior differences with the new engine.

* Add new optional extra variant for symengine for QPY

* Stop parsing sympy and symengine pulse expressions

During QPY deserialization of old payloads that include pulse schedules
we are unable to construct the pulse schedule from the payload. For
circuits that include pulse gates the strategy for loading those
payloads is to emit a warning that the pulse component will be omitted
and create the rest of the circuit like normal. To do this the qpy
deserialization has to read the size of the payload to seek to the right
position. However we don't need to actually parse the expression because
it's never used. This commit removes the parsing step it is
unnecessary after we do the read that will seek the cursor
appropriately.

* Update releasenotes/notes/parameter-expression-rs-6fddbb2556b7e1f7.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Remove unused arguments

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 22, 2025
With the move to symbolic expression class added in Qiskit#13278 the
dependency on sympy is no longer as core to the Qiskit package as it was
in the past. The only pieces that are still reliant on it are QPY
deserialization of payloads older than QPY v13 and some small
visualization utilities where sympy is used to generate the output.
Since this functionality is not core to the library and only used in
some less common use cases we no longer need to install sympy for every
installation of Qiskit. In preparation for this Qiskit 2.0 laid the
groundwork by marking all uses of sympy in Qiskit as being fallible if
sympy was not installed and documented that sympy is not guaranteed to
be installed with Qiskit anymore.

This commit implements this change and moves sympy to be an optional
dependency only used by some small components.
github-merge-queue bot pushed a commit that referenced this pull request Jun 2, 2025
* Remove sympy as a hard dependency

With the move to symbolic expression class added in #13278 the
dependency on sympy is no longer as core to the Qiskit package as it was
in the past. The only pieces that are still reliant on it are QPY
deserialization of payloads older than QPY v13 and some small
visualization utilities where sympy is used to generate the output.
Since this functionality is not core to the library and only used in
some less common use cases we no longer need to install sympy for every
installation of Qiskit. In preparation for this Qiskit 2.0 laid the
groundwork by marking all uses of sympy in Qiskit as being fallible if
sympy was not installed and documented that sympy is not guaranteed to
be installed with Qiskit anymore.

This commit implements this change and moves sympy to be an optional
dependency only used by some small components.

* Add sympy to backwards compat qpy jobs

* Update release notes

* Update releasenotes/notes/no-sympy-19687d5ca814042c.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
rahaman-quantum pushed a commit to rahaman-quantum/qiskit that referenced this pull request Jun 20, 2025
* Add SymbolicExExpression in Rust

This PR is a new implementation of ParameterExpression written in Rust for issue Qiskit#13267

This PR implements our own symbolic expression library written in Rust, this replaces
symengine from ParameterExpression class.

`crates/circuit/symbol_expr.rs` is a symbolic expression module. Equations with symbols
and values are expressed by using AST structure with nodes of operations. This module
contains all the operators defined in Python's ParameterExpression class.

To separate Rust's implementation from Python we prepared Python interface class
`crates/circuit/symbol_expr_py.rs` that is called from ParameterExpression in Python,
instead of symengine.

`crates/circuit/parameterexpression.rs` is the ParameterExpression class for Rust.
It provides similar functionalities to Python version.

* Remove unused parameterexpression.rs file

This file is more future facing and doesn't have any current uses. To
simplify the PR to just the working component this commit removes the
file. We can add it back in the future when there is use case for the
module.

* Change py suffix to prefix

This commit just renames the symbol_expr_py file and module to
py_symbol_expr. We typically use a Py prefix when describing objects
that are just for python. This change was just done to make this new
module match that expectation.

* Fix qiskit-circuit cargo dependencies

The Cargo.toml for qiskit-circuit was missing some details to use the
approx crate correctly with Complex64 types and arrays of those types.
This didn't bubble up in combined builds because cargo was reusing the
compilation unit for these crates with accelerate which had the correct
settings. But when trying to do a standalone build of qiskit-circuit
this caused a compilation error. This commit fixes this by updating the
local dependencies to ensure the appropriate features to use approx with
complex and ndarray are set.

* Use unix style newlines for symbol_parser.rs

* Remove unused extern crate lines

* Run cargo fmt

* add some missing fuction to Python interface

* fix sqrt

* add values function to enumerate all values in expression for pi_check.py

* fix for lint

* fix for lint error

* Make conjugate a unary op

In testing the conjugate was not working for parameters. This is because
it was not being treated as a unary op. Values were correctly changing
the sign of the complex component, but symbols and expressions would
not. To fix this the conjugate was added as a unary op which is tracked
added to the expression tree.

* Add test module for validating all parameter expression ops

This commit expands the test coverage for the parameter expression class
to ensure that we have coverage of all the mathemetical operations that
can be performed on an expression and does so with combinations of
operand types to try and provide more thorough coverage of the class.

The only method missing from the tet right now is gradient which can be
added later.

* Add tolerance on new unit tests for multiplication with complex bind

This commit adds tolerance to the floating point comparison on the
multiplication test with binding a complex number. Running these tests
in CI was showing failures on different platforms caused by floating
point precision differences. This should fix the failures in CI.

* Fix pow logic for complex numbers

The pow logic was overeagerly casting values to complex whenever the lhs of
the operation was negative. The output is only complex if the lhs is
negative and the rhs has a fractional component. This casting to a
complex was accumulating a small imaginary component because of
fp precision even though the values shouldn't be complex. This commit
fixes this issue by adjusting the pow logic to only cast a real or
integer value to a complex if it's negative and the rhs has a fractional
component.

* Fix more pow impl and expand coverage

* Remove unused pyfunctions

* Add more test coverage

* More test coverage

* Add tests for .numeric()

* Add tolerance to test_pow_complex_binding

This test was failing in CI on Windows due to differing fp precision. To
fix the failure this adjusts the tests to use assertAlmostEqual instead
of assertEqual to account for the platform differences.

* modify SymbolExpr enum structure

* Fix add/sub optimization

* Fix PartialOrd for Binary

* remove Symbol struct, add optimization function and remove on-the-fly optimization

* add optimization for values, fix equality check

* remove debug prints

* reformat for lint test

* fix cargo error

* fix for lint

* fix for lint

* Fix np.complex128 issue, remove codes for exceptions for np.complex128

* fix for lint

* fix for lint

* Fix symbol_parser.rs, remove BinaryOpContainer, reordering pow operator

* replace rhs to lhs in __r*__ functions, add multispace0 for unary op parser

* remove unused imports

* move replacements of backslashes for sympify into rust

* fix for lint

* remove enum for Python multi-types and replace them to PyObject

* optimize __eq__ when comparing with value

* remove optimmize() from __hash__

* rename PySymbolExpr to ParameterExpression and implemented some function so that we can use both from rust and python

* Fix log and div

* Expand dedicated ParameterExpression testing

* Remove unicode charcter to fix windows charmap failure

* recover value.py as Qiskit#14024

* lint

* reflect review comments

* cargo fmt

* remove unused imports

* fix cloning Values

* format

* add error handling on parse_expression

* add exception to is_real when there are unbound parameters

* Revert "add exception to is_real when there are unbound parameters"

This reverts commit 1c98127.

* fix derivative of asin/acos, raise error for sign/conj. Fix sympify with complex numbers

* restore test_pauli_feature_map, add comment for epsilon

* format

* fix for lint

* remove debug print

* format

* Additional tests and gradient note

- Added a note that the gradient of parameters
  is only defined for real parameter expressions
- Added a test to call SparsePauliOp.simplify on
  with coefficients that have complex multipliers
- Added numerical checks for derivatives

* Update crates/circuit/src/parameter_expression.rs

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update crates/circuit/src/parameter_expression.rs

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs / comments and fix conjugate deriv

- Update docs: we no longer rely on symengine/sympy for parameters
- Reword reno: we still install symengine/sympy at this point
- Fix conjugate derivative: since conj() acts as identity on real numbers, the derivative is 1

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
rahaman-quantum pushed a commit to rahaman-quantum/qiskit that referenced this pull request Jun 20, 2025
…14391)

* Avoid using string parsing for ParameterExpression.sympify()

In the recently merged Qiskit#13278 the implementation for the sympify
method for ParameterExpression was changed because we no longer rely on
symengine internally. Previously we would just return the inner
symengine object used to represent the symbolic expression. Without
symengine available Qiskit#13278 updated the implementation of the method to
generate a string representation of the expression and pass that to
sympify which has a "parser" for converting that expression string to a
sympy object. However, sympify() method is insecure as it internally
relies on Python's eval() to parse the string and can't be used for
untrusted input. While this doesn't have the same exact exposure as
in GHSA-6m2c-76ff-6vrf
because you have to opt-in to using this function with input that is
untrusted and the degrees of freedom are less because it has to go
through the rust symbolic expression it is still a potential
vulnerability waiting to happen. This commit reworks the sympify
implementation to avoid using sympy's parser and instead just builds
the sympy expression from the internal state directly.

* Remove unused Rust functions that support sympy string generation

* Add test coverage for all of parameter expression

* Add .sign() to the megaexpression
rahaman-quantum pushed a commit to rahaman-quantum/qiskit that referenced this pull request Jun 20, 2025
* Remove symengine from the requirements list

In Qiskit#13278 we removed our dependency on symengine and replaced it with
our own symbolic engine. However that PR neglected to remove symengine
from the requirements list. This commit corrects this oversight and
removes it from the requirements list. It also updates the release notes
to make them a bit more prescise and document the change in requirements
and potential behavior differences with the new engine.

* Add new optional extra variant for symengine for QPY

* Stop parsing sympy and symengine pulse expressions

During QPY deserialization of old payloads that include pulse schedules
we are unable to construct the pulse schedule from the payload. For
circuits that include pulse gates the strategy for loading those
payloads is to emit a warning that the pulse component will be omitted
and create the rest of the circuit like normal. To do this the qpy
deserialization has to read the size of the payload to seek to the right
position. However we don't need to actually parse the expression because
it's never used. This commit removes the parsing step it is
unnecessary after we do the read that will seek the cursor
appropriately.

* Update releasenotes/notes/parameter-expression-rs-6fddbb2556b7e1f7.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Remove unused arguments

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
rahaman-quantum pushed a commit to rahaman-quantum/qiskit that referenced this pull request Jun 20, 2025
* Remove sympy as a hard dependency

With the move to symbolic expression class added in Qiskit#13278 the
dependency on sympy is no longer as core to the Qiskit package as it was
in the past. The only pieces that are still reliant on it are QPY
deserialization of payloads older than QPY v13 and some small
visualization utilities where sympy is used to generate the output.
Since this functionality is not core to the library and only used in
some less common use cases we no longer need to install sympy for every
installation of Qiskit. In preparation for this Qiskit 2.0 laid the
groundwork by marking all uses of sympy in Qiskit as being fallible if
sympy was not installed and documented that sympy is not guaranteed to
be installed with Qiskit anymore.

This commit implements this change and moves sympy to be an optional
dependency only used by some small components.

* Add sympy to backwards compat qpy jobs

* Update release notes

* Update releasenotes/notes/no-sympy-19687d5ca814042c.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog 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.

API documentation ParameterExpression lacks the __*__ methods Oxidize ParameterExpression
9 participants