Skip to content

QPY: Correctly load ParameterExpression with empty replay#15876

Closed
gadial wants to merge 7 commits intoQiskit:mainfrom
gadial:fix_empty_parameter_expression_qpy
Closed

QPY: Correctly load ParameterExpression with empty replay#15876
gadial wants to merge 7 commits intoQiskit:mainfrom
gadial:fix_empty_parameter_expression_qpy

Conversation

@gadial
Copy link
Copy Markdown
Contributor

@gadial gadial commented Mar 25, 2026

Summary

Fixes a bug where ParameterExpression did not load correctly when the expression was "empty" (e.g. 0*x).

Fixes #15355

Details and comments

ParameterExpressions are stored using a "replay" which enables to reconstruct them step by step. An expression like 0*x reduces to an empty expression (considered as the value 0) with an empty replay - this crashes when trying to reconstruct the expression with the ParameterExpression::from_qpy method.

To solve this, explicit handling of the empty replay case was added. The subtle point is that the symbol table should be preserved, so the parameter list of the circuit will be as before even if those parameters were not used in practice. This was also simple to implement as the QPY file already has this data.

@gadial gadial requested a review from a team as a code owner March 25, 2026 15:21
@qiskit-bot
Copy link
Copy Markdown
Collaborator

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

  • @Qiskit/terra-core
  • @mtreinish

@mtreinish mtreinish added stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge. Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. labels Mar 25, 2026
@mtreinish mtreinish added this to the 2.4.0 milestone Mar 25, 2026
@mtreinish mtreinish added the mod: qpy Related to QPY serialization label Mar 25, 2026
@jakelishman jakelishman modified the milestones: 2.4.0, 2.4.0rc2 Mar 26, 2026
Copy link
Copy Markdown
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I tried this locally, and while it fixes all the instances listed in the test, I'm not convinced it's solving the true underlying issue, because you can have an expression with cancelled-out parameters that isn't 0 too:

import io
from qiskit import qpy, QuantumCircuit
from qiskit.circuit import Parameter

x = Parameter("x")
# Using either of these triggers the same bug.
bad = [0 * x + 2]

qc = QuantumCircuit(1)
qc.rz(bad[0], 0)

with io.BytesIO() as fptr:
    qpy.dump(qc, fptr)
    fptr.seek(0)
    x = qpy.load(fptr)[0]

This should satisfy x[0].params[0] == 2, but it's still coming out as 0 after this PR. Without this PR it fails with the same "empty replay" bug.

This feels like it might be a problem with the dump rather than the load?

Also: I'm getting a panic in qiskit_circuit::parameter::parameter_expression::ParameterExpression::from_qpy with these (without this PR) which we could do with turning into a safer exception too (but can be a follow-up).

@gadial
Copy link
Copy Markdown
Contributor Author

gadial commented Mar 27, 2026

This should satisfy x[0].params[0] == 2, but it's still coming out as 0 after this PR. Without this PR it fails with the same "empty replay" bug.

This feels like it might be a problem with the dump rather than the load?

Also: I'm getting a panic in qiskit_circuit::parameter::parameter_expression::ParameterExpression::from_qpy with these (without this PR) which we could do with turning into a safer exception too (but can be a follow-up).

This actually makes a lot of sense. The replay doesn't have x while the instruction still lists it as a parameter. So even in the case where we use the replay, we need to verify the symbol table is correct - I'll fix this.

EDIT: after playing with this a little, the problem is more serious than I thought, and it's present even in the Python implementation. 0*x+2 reduces to the value 2, which in our current rust implementation for parameter expressions has an empty replay. Obviously, loading as the value 0 is incorrect here, so we need a better mechanism altogether to store a parameter expression with an empty replay. We can't simply store it as value since we need to store the symbol table as well, to account for the unused parameters. And in QPY17, we don't have an op for constants.

The workaround I suggest is saving the constant value a as the parameter expression a+0, i.e. use an ADD operand. This will simplify out when reconstructing the expression, and will enable us to store parameter information. This should be added to the QPY18 TODO pile, since this is obviously not a perfect solution.

gadial and others added 5 commits March 27, 2026 15:51
* Enable missing argument in docstring lint

In the recently merged Qiskit#15603 we migrated from pylint to use
ruff. During the development of that PR the rule group "D" for rules
derived from pydocstyle was investigated but the full rule group was
overly pedantic and even with disabling the most egregious rules also
changed the docs for the worse (at least by autofixing many rules) so
that sphinx would not succesfully build after applying the fixes. So
during Qiskit#15603 the changes for this rule group were reverted. However,
one of the useful rules from that group D417 which checks that all
arguments are documented. This rule found real issues in the
documentation and it is worthwhile to enable it. This commit enables the
rule in our ruff checks and fixes the issues associated with.

During this process ruff flagged one place in the `__call__` method
docstring for the `TwoQubitControlledUDecomposer` class has not
documented several arguments on the method. However, these arguments
are unused currently. It is apparently trying to match the `__call__`
signature of the other two qubit decomposers but is not using most of
the arguments. Most could be conceivably supported by
`TwoQubitControlledUDecomposer` and it's not clear why they were added.
But, for this PR I didn't want to look into adding the support as it was
out of scope. For the time being this PR suppresses the ruff rule and
adds a TODO comment to start using the unused arguments.

* Update filename docstring

* Revise wording on state visualization filename arg
…t#15881)

In the recently merged Qiskit#15871 we updated the consolidate blocks pass to
use Matrix4 in the common path of a 2q block being consolidated. This is
like > 90% of what the pass does when run in the preset pass manager.
However, there were uncommon cases in the pass around the handling of
blocks of a single gate that are outside of the target which were not
updated to use nalgebra arrays if it's a fixed size 1q or 2q gate. This
commit updates these uncommon paths so that we're always returning an
nalgebra matrix in the output UnitaryGate if the block being consolidated
is a single qubit or two qubits.
@gadial gadial requested a review from a team as a code owner March 29, 2026 12:22
@gadial
Copy link
Copy Markdown
Contributor Author

gadial commented Mar 31, 2026

Done in #15900

@gadial gadial closed this Mar 31, 2026
@github-project-automation github-project-automation Bot moved this from Ready to Done in Qiskit 2.4 Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. mod: qpy Related to QPY serialization stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

QPY fails to roundtrip expressions with cancelled-out variables

4 participants