Add some compatibility fixes to SymbolExpr #14508
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 15468871640Details
💛 - Coveralls |
| @@ -0,0 +1,9 @@ | |||
| --- | |||
| fixes: | |||
There was a problem hiding this comment.
We don't actually need a release note for this. We haven't released 2.1.0 yet so there hasn't been a release with SymbolExpr yet.
| alpha = Parameter(r"\alpha") | ||
| beta = Parameter(r"\beta") | ||
| qc.rz(alpha + beta, 0) | ||
| self.assert_roundtrip_equal(qc) |
There was a problem hiding this comment.
Can you add tests to validate the other fixes you made?
There was a problem hiding this comment.
I added 2 test cases to test 2 integer fraction add/sub optimization
There was a problem hiding this comment.
I'm not sure I understand the need for Value::Fraction here. We already have the Div op, how does fraction differ from that. I'm tempted to say we should maybe revert 08e830e from this PR and discuss that in isolation since it does seem unrelated to the fixes for the issues reported in #14467.
I am also still worried the test coverage here is a bit low. We're adding a new value type internally so we should try to ensure this is fully exercised. We should try to ensure we have testing coverage for this path in some of the other paths. Specifically I'm thinking the string formatting and ParameterExpression.sympify
crates/circuit/src/symbol_parser.rs
Outdated
| @@ -106,6 +102,9 @@ fn parse_unary(s: &str) -> IResult<&str, SymbolExpr, VerboseError<&str>> { | |||
| "log" => UnaryOp::Log, | |||
| "exp" => UnaryOp::Exp, | |||
| "sign" => UnaryOp::Sign, | |||
| "conj" => UnaryOp::Conj, // this is for backward compatibility | |||
There was a problem hiding this comment.
Backwards compatibility with what? Between Qiskit and sympy or something else. If it's just with earlier versions of this PR I don't think we need this because it's never been released.
There was a problem hiding this comment.
Without Value::Fraction I was feeling it is difficult to optimize only by BinaryOp::Div. For example, we could not calculate a/3 + 2*a/3 as a without Value::Fractional because 2*a/3 is represented by 2 Binary ops, Binary(BinaryOp:Div, Binary(BinaryOp::Mul, 2, a), 3).
By using Value::Fraction it is easy to understand we can add 2 binaries; Binary(BinaryOp::Mul, 1/3, a) + Binary(BinaryOp::Mul, 2/3, a)
I think we can separate this issue into other PR, but this issue is also urgent should be fixed before Qiskit 2.1
conj is used in previous SymbolExpr PR, but is not released yet so can we just replace as conjugate ?
There was a problem hiding this comment.
I removed the Fraction piece from this, the equality operator matching division expressions that are mathematically equivalent isn't super critical for 2.1, in general __eq__ doesn't handle equivalent expressions anyway so I'm fine with that as a known limitation. I worried more about adding complexity to the symbol expr code at this point in the release. We can look at adding that in 2.2. Without that I think this lgtm I added a small test to test backslashes with $ but otherwise it's just a rebase to merge.
* Add fixes to SymbolExpr * format * format * add comment on test * Add test cases for optimization of integer fraction * implement Value::Fraction to optimize integer expressions * format * Revert "format" This reverts commit 9abb5ec. * Revert "implement Value::Fraction to optimize integer expressions" This reverts commit 08e830e. * Remove conj we don't have backwards compatibility with earlier name * Add dedicated test for math mode with latex style parameter names --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
This pr fixes #14467 and some issues for compatibility
Details and comments
For #14467, SymbolExpr now accepts back slash when parsing string for
ParameterExpression.init.Previously SymbolExpr accepts
_,$\,[,]for a name of Parameter, now accepting_,$,\,[,]Implement
Value::Fractionto optimize division by integer instead of converting toValue::Realbefore, for examplea/2was converted to0.5*a. So we can now optimize equation with integer fraction generally.For example,
a/4 + 3*a/4is optimized asaora/2 - 0.5*ais optimized as0.Fix displaying conjugate op as
conjugateas similar to sympy instead ofconjoutput before.