Remove QPY warning on deserialising incomplete ParameterVectors#16222
Conversation
d14a9dd to
8e4b783
Compare
Coverage Report for CI Build 26823200940Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.07%) to 87.563%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions2761 previously-covered lines in 91 files lost coverage.
Coverage Stats
💛 - Coveralls |
This warning was added with QPY v3[^1] at a time when `ParameterVector` generated all its elements with unrelated UUIDs. Since `qiskit-terra` v0.25, we actually _have_ had enough information to completely recreate an entire vector from a single element[^2], and the circumstances needed to observe the incomplete behaviour are particularly convoluted, and unlikely in practice. [^1]: 5c4cd2b: Fix ParameterVector handling in QPY serialization (Qiskitgh-7381) [^2]: 5ab231d: Slightly optimize ParameterVector construction (Qiskitgh-10403)
8e4b783 to
41a35f5
Compare
|
One or more of the following people are relevant to this code:
|
There was a problem hiding this comment.
Thanks Jake. This PR is a bit out of my comfort zone, but that was the point when we agreed on round robin.
AI/LLM disclaimer: I have first carefully looked at the changed myself, and then asked my new friend Claude to go over the changes together, so LGTM!
Though one general question: why was it necessary to do the change both in python and in rust: wouldn't just changing the rust code be enough?
And one typo (detected both by me and Claude).
Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
The Python code is still live code - the Rust reader only handles QPY >= v13. If it weren't live, we'd have deleted it from the repo. |
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks for the quick reply!
This warning was added with QPY v31 at a time when
ParameterVectorgenerated all its elements with unrelated UUIDs. Sinceqiskit-terrav0.25, we actually have had enough information to completely recreate an entire vector from a single element2, and the circumstances needed to observe the incomplete behaviour are particularly convoluted, and unlikely in practice.Based on #16221.
The circumstances you can observe the bad behaviour are in the release note - it's pretty convoluted once #16221 is in place. I've separated off this PR from the others to discuss whether we truly want this. I think, with #16221 merged, the warning is no longer serving a meaningful purpose, and it costs us to maintain the tracking for it.
AI/LLM disclosure
Footnotes
5c4cd2b: Fix ParameterVector handling in QPY serialization (Fix ParameterVector handling in QPY serialization #7381) ↩
5ab231d: Slightly optimize ParameterVector construction (Slightly optimize ParameterVector construction #10403) ↩