[WIP] Fix QPY fails expressions with cancelled-out variables #15355 #15397
Conversation
qiskit-bot
added
the
Community PR
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
|
|
| match &expr.expr { | ||
| SymbolExpr::Value(_) | SymbolExpr::Symbol(_) => { | ||
| // We generate a POS (identity) operation for Values and Symbols for expressions with all cancelled-out variables | ||
| let lhs_value = ParameterValueType::extract_from_expr(&expr.expr); | ||
| replay.push(OPReplay { | ||
| op: OpCode::POS, | ||
| lhs: lhs_value, | ||
| rhs: None, | ||
| }); | ||
| } | ||
| _ => qpy_replay_helper(expr, name_map, replay), | ||
| } | ||
| } | ||
|
|
||
| fn qpy_replay_helper( | ||
| expr: &ParameterExpression, | ||
| name_map: &HashMap<String, Symbol>, | ||
| replay: &mut Vec<OPReplay>, |
There was a problem hiding this comment.
Is the expression simplification step always guaranteed to turn a “cancelled‑out” expression into a plain Value or Symbol before qpy_replay runs, or is it possible that qpy_replay_helper still receives a more complicated expression tree that, mathematically, is just a constant (for example, several nested adds and multiplies that reduce to zero)?
If that second case can happen, would it be safer for qpy_replay_helper to check whether filter_name_map has removed all parameters from the expression and, in that situation, emit a single POS replay step so that all constant‑after‑cancellation expressions are treated the same way during QPY replay?
There was a problem hiding this comment.
The proposed qpy_replay_helper is the old qpy_replay, it didn't change. It is a recursive function. In this case we have special case for the first iteration. The whole issue is that replay generates a list of operations, and the POS operation is just a way of generating a fake operation for representing a single value.
In the case you are describing is not an issue. Consider the following code
x = Parameter("x")
y = Parameter("y")
qc.rz(1 + y + 0 * x, 0)the code will generate and ADD operation with 1 and y as operands. X will be discarded.
|
I was thinking whether every “zeroed‑out” parameter expression is always simplified to a plain constant before saving to QPY, or if the helper should defensively treat any expression that ends up with no parameters as a constant too. Please check the INLINE comment for details. |
|
I am trying to address an specific problem of serializing Expressions that have evaluated to a single value. If you check the code in |
4 participants
Summary
[WIP] This PR fixes partially #15355, but I would like to get feedback before finishing it.
This PR fixes QPY round-tripping of certain ParameterExpression objects where Parameters are eliminated during internal expression simplifications. QPY serialization currently relies exclusively on binary or unary operator representation of expressions. When an expression such as
0 * xorx - xis simplified to a constant before serialization, QPY fails to store the Parameter Expression.Details and comments
This PR modifies the recursive replay logic in parameter_expression.rs: During the first replay step, if the expression consists of a Variable or Symbol and no operations, we now inject an explicit
OpCode::POS(Python’s unary positive operator, acting as an identity). This ensures all serialized ParameterExpression objects conform to the operator-based representation expected by QPY.This approach maintains QPY’s current structural conventions, avoids breaking compatibility, and ensures correct roundtripping for expressions that previously failed.
Remaining Work
Unused parameters are not preserved when loading a circuit. I plan to address this in a follow-up PR once the approach in this one is approved.