-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 15065615407Details
💛 - Coveralls |
@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. |
For circuits, real is enough. But we currently also allow parameter expressions in the |
crates/circuit/src/symbol_expr.rs
Outdated
_ => false, | ||
}, | ||
SymbolExpr::Unary(e) => e.expr.is_complex(), | ||
SymbolExpr::Binary(e) => e.lhs.is_complex() || e.rhs.is_complex(), |
There was a problem hiding this comment.
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
)
I just wanted to point out that not all of the |
crates/circuit/src/symbol_expr.rs
Outdated
pub fn complex(&self) -> Option<Complex64> { | ||
match self.eval(true) { | ||
Some(v) => match v { | ||
Value::Real(_) => Some(0.0.into()), |
There was a problem hiding this comment.
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 ?
crates/circuit/src/symbol_expr.rs
Outdated
} | ||
|
||
pub fn rcp(self) -> SymbolExpr { | ||
match self { |
There was a problem hiding this comment.
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?
crates/circuit/src/symbol_expr.rs
Outdated
Value::Complex(e) => Value::Complex(e.ln()), | ||
} | ||
} | ||
pub fn sqrt(&self) -> Value { |
There was a problem hiding this comment.
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
crates/circuit/src/symbol_expr.rs
Outdated
} | ||
pub fn sqrt(&self) -> Value { | ||
match self { | ||
Value::Real(e) => Value::Real(e.sqrt()), |
There was a problem hiding this comment.
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?
crates/circuit/src/symbol_expr.rs
Outdated
} | ||
} | ||
|
||
pub fn is_minus(&self) -> bool { |
There was a problem hiding this comment.
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?
I'm trying to fix errors in test cases but I'm feeling difficulty to solve complicated test cases (e.g. |
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
… into ParameterExpression_rust
There was a problem hiding this 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).
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.
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.
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.
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.
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.
* 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
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.
* 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>
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.
* 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>
* 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>
…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
* 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>
* 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>
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)