Fix QPY deserialization of UCRX, UCRY, and UCRZ gates #7933
Conversation
This commit fixes the deserialization of the UCRX, UCRY, and UCRZ gates. These gates aren't constructed in the standard way as other gate objects where the parameters are passed directly as args to the class constructor. Instead they behave the same as Initialize and the params are passed as a list to the constructor to make a new gate object. This commit fixes the handling in deserialization to treat these gates the same as Initialize so that it can be properly deserialized by qpy.load(). Fixes Qiskit#7847
mtreinish
added
stable backport potential
|
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 the following people are requested to review this:
|
| issues: | ||
| - | | ||
| QPY deserialization with the :func:`.qpy.load` function of a directly | ||
| instantiated :class:`~.UCPauliRotGate` object in a circuit will fail |
There was a problem hiding this comment.
Is there a long term fix for supporting UCPauliRotGate? And do you know if this gate class is used anywhere in Qiskit, if at all?
There was a problem hiding this comment.
The only place I've seen it directly used is by UCRX, UCRY, and UCRZ as the parent class. It's basically just a base class for the axis specific versions to de-duplicate the decomposition of the gate. As to how to fix it the only way I can think to is to bump the qpy version to v5 and add a field to the custom instruction type for UCPauliRotGate to store the rotation angle as an extra field in the payload. There is also the hacky equivalent where we modify _write_instruction to add the rotation axis to the parameter list for a UCPauliRotGate and handle that in a custom path on deserialization but that feels like the wrong way to handle this. Ideally we'd refactor all these classes to store the angle list as a single parameter and then for UCPauliRotGate we have a second parameter for the rotation axis, but that's a larger refactor.
Pull Request Test Coverage Report for Build 2168569415
💛 - Coveralls |
There was a problem hiding this comment.
In order to completely fixed this issue for sending the circuit to IBM Cloud / IBM Quantum, we also need to update the QPY version on the respective provider as well, don't we? Qiskit/qiskit-ibm-runtime#250 (comment)
Generally, yes to fix an issue in submitting the circuit to the qiskit runtime service you'll need to backport this fix to the qiskit-ibm-runtime package's fork of qpy. However, in this case I don't think it is necessary because deserialization is done server side and the error would occur in the qiskit-runtime service's environment itself and the not the client when it tried to load the qpy file. When the runtime environment updates their qiskit version to 0.20.1 with this fix I would expect it to just start working because there is no change in the file format as part of this fix. |
|
Sigh, it looks like @nkanazawa1989's approval isn't enough despite being a codeowner for qpy because the tests for qpy don't live in a path that matches qpy/ in the codeowners file. We should move the tests to fix this in the future. |
|
@Mergifyio requeue |
☑️ This pull request is already queued |
|
@Mergifyio refresh |
✅ Pull request refreshed |
This commit fixes the deserialization of the UCRX, UCRY, and UCRZ gates. These gates aren't constructed in the standard way as other gate objects where the parameters are passed directly as args to the class constructor. Instead they behave the same as Initialize and the params are passed as a list to the constructor to make a new gate object. This commit fixes the handling in deserialization to treat these gates the same as Initialize so that it can be properly deserialized by qpy.load(). Fixes #7847 Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 2f182f6)
|
Oh duh, it wasn't re-evaluating because it didn't have the automerge label. |
This commit fixes the deserialization of the UCRX, UCRY, and UCRZ gates. These gates aren't constructed in the standard way as other gate objects where the parameters are passed directly as args to the class constructor. Instead they behave the same as Initialize and the params are passed as a list to the constructor to make a new gate object. This commit fixes the handling in deserialization to treat these gates the same as Initialize so that it can be properly deserialized by qpy.load(). Fixes #7847 Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 2f182f6) Co-authored-by: Matthew Treinish <[email protected]>
Summary
This commit fixes the deserialization of the UCRX, UCRY, and UCRZ gates.
These gates aren't constructed in the standard way as other gate objects
where the parameters are passed directly as args to the class
constructor. Instead they behave the same as Initialize and the params
are passed as a list to the constructor to make a new gate object. This
commit fixes the handling in deserialization to treat these gates the
same as Initialize so that it can be properly deserialized by
qpy.load().
Details and comments
Fixes #7847