Add ParameterExpression in Rust #13278
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13718407501Details
💛 - 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 |
1ucian0
added
the
Rust
crates/circuit/src/symbol_expr.rs
Outdated
| rhs : SymbolExpr, | ||
| } | ||
|
|
||
| impl Clone for SymbolExpr { |
There was a problem hiding this comment.
Why do we need this separate Clone implementation? It looks like it would be the same as the standard Clone, but maybe I'm missing something. Or is this doing something specific to the Arc in Unary and Binary?
There was a problem hiding this comment.
I think it is ok to remove Clone and just deriving Clone
crates/circuit/src/symbol_expr.rs
Outdated
| Value::Real(r) => r, | ||
| Value::Complex(c) => c.re, | ||
| } | ||
| None => 0.0, |
There was a problem hiding this comment.
I might not understand this fully, but if self is a plain Symbol then self.eval returns None and this the functions real/complex will return 0, right? But if I just have a plain Symbol with no value associated yet, then I would expect that I cannot query the real or complex parts, because they are not defined, no? (In which case I thought this might need to raise an error 🙂)
There was a problem hiding this comment.
I changed return type to Option
crates/circuit/src/symbol_expr.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn has_symbol(&self, param: String) -> bool { |
There was a problem hiding this comment.
Should we use &String or even &str here since we're only reading the value?
There was a problem hiding this comment.
I changed them to reference of String
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.
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)
crates/circuit/src/symbol_expr.rs
Outdated
| match self.lhs.eval(recurse) { | ||
| Some(v) => lval = v, | ||
| None => return None, | ||
| } | ||
| match self.rhs.eval(recurse) { | ||
| Some(v) => rval = v, | ||
| None => return None, | ||
| } |
There was a problem hiding this comment.
You could make this a bit less typing by matching the tuple if you would like (no guarantees on the formatting 🙂)
| match self.lhs.eval(recurse) { | |
| Some(v) => lval = v, | |
| None => return None, | |
| } | |
| match self.rhs.eval(recurse) { | |
| Some(v) => rval = v, | |
| None => return None, | |
| } | |
| match (self.lhs.eval(true), self.rhs.eval(true)) { | |
| (Some(left), Some(right)) => { | |
| lval = left; | |
| rval = right; | |
| } | |
| _ => return None, | |
| } |
doichanj
requested review from
a team,
nonhermitian,
eggerdj,
wshanks,
alexanderivrii and
jyu00
as code owners
|
I just wanted to point out that not all of the |
ShellyGarion
removed request for
wshanks,
nonhermitian,
alexanderivrii,
jyu00 and
eggerdj
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.
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 { |
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.
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.
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.
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. |
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.
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.
|
I expanded the test coverage a bit in: 29a77a1 to ensure we had more complete coverage of the Python Locally I'm hitting 3 failures around the |
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.
In addition to making symengine optional Qiskit#13278 should also enable us to make sympy optional. It would only be potentially needed for loading QPY payloads that were generated using sympy encoding, but also some visualization functions. This commit lays the groundwork for removing it as a dependency by outlining the API changes for the functions that will optionally depend on sympy and raise an exception if it's not installed. It won't be possible to trigger the exception with sympy in the requirements list, but when we do remove it the API is prepared for it.
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.
This should be fixed now by: af6029d |
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.
There was a problem hiding this comment.
Thanks everyone for the work on this, I know it's been a huge effort.
Top level comments:
- we need to fix the Python/Rust boundary crossing of
np.complex128. There's way too much special handling all over the place, and it impliesisinstancechecks are necessary in Python space all over the place, often ones that would then immediately need to be repeated by Rust space during extraction. - Do we have performance numbers for this?
- I'm concerned that the on-the-fly simplifications of expressions are nowhere near as complete as they are in
sympyandsymengine, which may well have significant performance and memory impacts for users using these heavily. For example, givena = Parameter("a"), doinga + a - awill not returna, it'll return-a + 2*a. I think this is indicative that what we're replacingsymengine/sympywith is not a CAS of the same caliber, even for the limited subset of operations we support. That said, if the only operations we care about are really really simple, it might be cheaper to simply not do the symbolic substitutions (as we currently aren't). But that will be disastrous for performance with complicated expressions, which are often reliant on intermediate cancellation being done all the time, if not only to canonicalise the results. - I'm concerned that the numerical evaluation, particularly in floating-point contexts, is introducing a lot of fuzzy comparisons, often to the
f64::EPSILON, even when that value is not known to be of the correct scale. I'm concerned that the evaluators are introducing quite a lot of round-off error at many different levels of the execution, and that's going to cause quite a lot of pain if this merges - we already have pain withsymenginebeing slightly lax sometimes with its complex handling, compared to sympy which is usually very precise.
I stopped reading at the point of my last comment in symbol_expr, and I haven't looked at the tests, because I need to go to bed.
| pub mod py_symbol_expr; | ||
| pub mod slice; | ||
| pub mod symbol_expr; | ||
| pub mod symbol_parser; |
There was a problem hiding this comment.
Could these three new ones go in a submodule, with a more clearly defined interface to the rest of the crate?
| def is_real(self): | ||
| """Return whether the expression is real""" | ||
| if not self._symbol_expr.is_real and self._symbol_expr.is_real is not None: | ||
| # Symengine returns false for is_real on the expression if | ||
| # returns false for is_real on the expression if | ||
| # there is a imaginary component (even if that component is 0), | ||
| # but the parameter will evaluate as real. Check that if the | ||
| # expression's is_real attribute returns false that we have a | ||
| # non-zero imaginary | ||
| if self._symbol_expr.imag == 0.0: | ||
| if self._symbol_expr.complex().imag == 0.0: | ||
| return True | ||
| return False | ||
| return self._symbol_expr.is_real |
There was a problem hiding this comment.
Much of the dancing around in here was because of awkwardnesses in symengine. Probably the whole initial if block can be removed, and we just use self._symbol_expr.is_real (which tbh, should probably be a function call).
| if self._symbol_expr.is_complex: | ||
| return self._symbol_expr.complex() | ||
| if self._symbol_expr.is_int: | ||
| return self._symbol_expr.int() | ||
| return self._symbol_expr.float() |
There was a problem hiding this comment.
The terminology here is a bit funny - I suspect is_complex might be better as !is_real. I think is_complex (at least looks like) a type-level question (is this represented internally by a Complex64?) and not a value-level question (can this be represented by a Complex64? - which would always be True for numeric types).
| try: | ||
| _ = float(pi) | ||
| except (ValueError, TypeError): | ||
| from sympy import sstr | ||
| import qiskit._accelerate.circuit |
There was a problem hiding this comment.
Does this still need to be a protected import?
| if _optionals.HAS_SYMENGINE: | ||
| import symengine | ||
|
|
||
| # Converts Sympy expressions to Symengine ones. | ||
| to_native_symbolic = symengine.sympify | ||
| else: | ||
| # Our native form is sympy, so we don't need to do anything. | ||
| to_native_symbolic = lambda x: x | ||
| # Our native form is sympy, so we don't need to do anything. | ||
| to_native_symbolic = lambda x: x |
There was a problem hiding this comment.
I'm pretty sure this is not true any more. Something looks funny about this file - it uses sympy internally, and needs to get back to Qiskit's native form, which is what this lambda originally did, but now our native form is neither.
crates/circuit/src/symbol_expr.rs
Outdated
| /// definition of unary operations | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub enum UnaryOps { | ||
| Abs, | ||
| Neg, | ||
| Sin, | ||
| Asin, | ||
| Cos, | ||
| Acos, | ||
| Tan, | ||
| Atan, | ||
| Exp, | ||
| Log, | ||
| Sign, | ||
| Conj, | ||
| } | ||
|
|
||
| /// definition of binary operations | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub enum BinaryOps { | ||
| Add, | ||
| Sub, | ||
| Mul, | ||
| Div, | ||
| Pow, | ||
| } |
There was a problem hiding this comment.
Minor point of naming: the enum should be a singular, since each member is a single value in the type. So it's UnaryOp::Abs, not UnaryOps::Abs.
crates/circuit/src/symbol_expr.rs
Outdated
| /// node types of expression tree | ||
| #[derive(Debug, Clone)] | ||
| pub enum SymbolExpr { | ||
| Symbol(Symbol), | ||
| Value(Value), | ||
| Unary(Arc<Unary>), | ||
| Binary(Arc<Binary>), | ||
| } | ||
|
|
||
| /// symbol with its name | ||
| #[derive(Debug, Clone)] | ||
| pub struct Symbol { | ||
| name: String, | ||
| } | ||
|
|
||
| /// Value type, can be integer, real or complex number | ||
| #[derive(Debug, Clone)] | ||
| pub enum Value { | ||
| Real(f64), | ||
| Int(i64), | ||
| Complex(Complex64), | ||
| } | ||
|
|
||
| /// unary operation node has 1 branch node | ||
| #[derive(Debug, Clone)] | ||
| pub struct Unary { | ||
| op: UnaryOps, | ||
| expr: SymbolExpr, | ||
| } | ||
|
|
||
| /// binary operation node has 2 branch nodes | ||
| #[derive(Debug, Clone)] | ||
| pub struct Binary { | ||
| op: BinaryOps, | ||
| lhs: SymbolExpr, | ||
| rhs: SymbolExpr, | ||
| } |
There was a problem hiding this comment.
This way of deeply nesting things feels a bit verbose to me, and the indirection feels like it's in a slightly weird place (at the top in SymbolExpr, not at the points of use in Binary, etc).
With our current model, which isn't doing anything like on-the-fly common subexpression elimination, I'm not sure that the Arc pointers are ever increfing beyond 1, so I think they could just be Box. That said, I think the indirection is in the wrong place here, and will be causing more copies around the heap than are actually necessary, due to Binary and Unary.
I think there's a few things going on:
Valueis already 3 pointers wide, of which an entire pointer (minus two bits) is padding (thoughSymbolas written requires it to be 3 pointers too).- ... so
SymbolExpris 4 pointers wide at least too. - The indirection in
BinaryandUnaryis in the wrong place: rather than indirecting the entirestruct Binary, we could indirect the individual expressions in it, so that we're always just dealing withArc<SymbolExpr>(though it's not clear to me that we need to be cloning all these things, andBoxmight be better).
If we don't need to be deep-cloning everywhere, I might suggest
pub enum SymbolExpr {
Symbol(Box<str>),
Value(Value),
Unary {
op: UnaryOp,
expr: Box<SymbolExpr>,
},
Binary {
op: BinaryOp,
lhs: Box<SymbolExpr>,
rhs: Box<SymbolExpr>,
}
}might be less verbose to work with, and it shouldn't be any larger in size.
I suppose it does depend a bit on how we construct things - if we really don't do any sharing or manipulation of expressions, then it might make sense to use something more like the current form, but it will be more verbose.
There was a problem hiding this comment.
Most importantly, perhaps, I think the current structure of separating out the Symbol, Unary and Binary objects into separate things with their own impl is leading to quite a bit more duplicated code, and it's rather hard to follow: the impl blocks are scattered everywhere, and almost everything in those methods still needs to be aware of all the other enum variants in SymbolExpr, so it's not really achieving much isolation, I think.
crates/circuit/src/symbol_expr.rs
Outdated
| /// Add with heuristic optimization | ||
| fn add_opt(&self, rhs: &SymbolExpr) -> Option<SymbolExpr> { | ||
| if self.is_zero() { | ||
| Some(rhs.clone()) | ||
| } else if rhs.is_zero() { | ||
| Some(self.clone()) | ||
| } else { | ||
| // if neg operation, call sub_opt | ||
| if let SymbolExpr::Unary(r) = rhs { | ||
| if let UnaryOps::Neg = r.op { | ||
| return self.sub_opt(&r.expr); | ||
| } | ||
| } | ||
|
|
||
| match self { | ||
| SymbolExpr::Value(e) => e.add_opt(rhs), | ||
| SymbolExpr::Symbol(e) => e.add_opt(rhs), | ||
| SymbolExpr::Unary(e) => match e.add_opt(rhs) { | ||
| Some(opt) => Some(opt), | ||
| None => match rhs { | ||
| // swap nodes by sorting rule | ||
| SymbolExpr::Binary(r) => { | ||
| if let BinaryOps::Mul | BinaryOps::Div | BinaryOps::Pow = r.op { | ||
| if rhs < self { | ||
| Some(_add(rhs.clone(), self.clone())) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| _ => { | ||
| if rhs < self { | ||
| Some(_add(rhs.clone(), self.clone())) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| }, | ||
| }, | ||
| SymbolExpr::Binary(l) => match l.add_opt(rhs) { | ||
| Some(opt) => Some(opt), | ||
| None => { | ||
| if let BinaryOps::Mul | BinaryOps::Div | BinaryOps::Pow = l.op { | ||
| // swap nodes by sorting rule | ||
| match rhs { | ||
| SymbolExpr::Binary(r) => { | ||
| if let BinaryOps::Mul | BinaryOps::Div | BinaryOps::Pow = r.op { | ||
| if rhs < self { | ||
| Some(_add(rhs.clone(), self.clone())) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| _ => { | ||
| if rhs < self { | ||
| Some(_add(rhs.clone(), self.clone())) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The way the SymbolExpr is represented is really not amenable to be doing this - this is exactly why things like Add are usually variadic operations in CASs (e.g. Mathematica, sympy, symengine) - you can assert a canonical sorting order, then you have the entire addition context available to you at once, and you don't need to go searching deep in the tree.
I'm concerned that this stuff might scale quadratically or worse with expression size, which is something variadics and a canonical ordering help avoid.
crates/circuit/src/symbol_expr.rs
Outdated
| pub fn sqrt(&self) -> Value { | ||
| match self { | ||
| Value::Real(e) => { | ||
| if *e < 0.0 { | ||
| Value::Complex(Complex64::from(e).powf(0.5)) | ||
| } else { | ||
| Value::Real(e.sqrt()) | ||
| } | ||
| } | ||
| Value::Int(e) => { | ||
| if *e < 0 { | ||
| Value::Complex(Complex64::from(*e as f64).powf(0.5)) | ||
| } else { | ||
| let t = (*e as f64).sqrt(); | ||
| let d = t.floor() - t; | ||
| if (-f64::EPSILON..f64::EPSILON).contains(&d) { | ||
| Value::Int(t as i64) | ||
| } else { | ||
| Value::Real(t) | ||
| } | ||
| } | ||
| } | ||
| Value::Complex(e) => { | ||
| let t = Value::Complex(e.sqrt()); | ||
| match t.opt_complex() { | ||
| Some(v) => v, | ||
| None => t, | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
CASs don't usually evaluate things like the square root quite so eagerly in integer contexts. I suppose it probably doesn't matter so much for us, because we don't use integer contexts much at all, but it's indicative of more behavioural differences.
crates/circuit/src/symbol_expr.rs
Outdated
| } else { | ||
| let t = (*e as f64).sqrt(); | ||
| let d = t.floor() - t; | ||
| if (-f64::EPSILON..f64::EPSILON).contains(&d) { | ||
| Value::Int(t as i64) | ||
| } else { | ||
| Value::Real(t) |
There was a problem hiding this comment.
This isn't really correct floating-point handling. The floating-point epsilon is very much a relative tool: it's very specifically just the smallest floating-point number such that 1. + e != 1.. It's meaningless if t is not close to 1, and even if it is, the number of floating-point values in [t, t + e) and (t - e, t] is not the same if t is a power of two, and I'm not certain what the test is asserting here. If it's meant to be relative, then presumably what it actually wants is something related to the ULP difference between the two numbers, though against ofc there's a bunch of subnormal / infinite / NaN handling that may need to be done.
(Also, it's using a half-open range for the comparison, so it's asymmetric in another way too.)
|
I noticed my implementation is very slow compared to main branch. I compared using following benchmark. This graph shows time to build parameterized equation (tparam in the code) This graph shows time to bind values to the parameterized equation (tbind in the code) |
|
Given @jakelishman's review and the performance issues @doichanj's identified I've deferred this to 2.1 since it's not going to be ready in time for the 2.0 release. We can circle back and discuss the direction here once 2.0rc1 is out the door and the dust has settled a bit on that release. |
* Document that symengine is optional for qpy.load This commit changes the API for qpy.load() to make it explicit that symengine may not be installed in the future but is a requirement for deserializing certain payloads that were generated using symengine. We're expecting to drop symengine as a requirement with #13278 which should improve Qiskit's installation story and also will facilitate adding a C api for circuits. But the qpy v10, v11, and v13 have a hard requirement on symengine (specifically 0.11.0 and 0.13.0) to be able to deserialize `ParameterExpression` if the symbolic encoding was set to use it. Since the function needs to support being able to deserialize these payloads symengine will need to be installed. This commit adds the document and release note to indicate this change in behavior. * Raise QPY compatibility version to remove symengine and sympy dependency This commit raises the qpy compatibility version to QPY format version 13. As the larger PR is planning for a world without symengine installed by default this becomes a problem for the generation side too. This becomes a blocker for using QPY < 13 in the future so this commit opts to just raise the minimum verison to get ahead of any potential issues. * Add HAS_SYMENGINE check on load function * Correct logic for ucr*_dg gates name changing * Add feature string to optional decorator * Prepare for an optional sympy In addition to making symengine optional #13278 should also enable us to make sympy optional. It would only be potentially needed for loading QPY payloads that were generated using sympy encoding, but also some visualization functions. This commit lays the groundwork for removing it as a dependency by outlining the API changes for the functions that will optionally depend on sympy and raise an exception if it's not installed. It won't be possible to trigger the exception with sympy in the requirements list, but when we do remove it the API is prepared for it. * Fix release note categorization * Add missing optional on template substitution
|
Now the performance issue is almost solved. |
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
ParameterExpressionclass.crates/circuit/symbol_expr.rsis 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.rsthat is called from ParameterExpression in Python, instead of symengine.crates/circuit/parameterexpression.rsis the ParameterExpression class for Rust. It provides similar functionalities to Python version. (but maybe we have to add more)