Fix ParameterVector handling in QPY serialization #7381

jakelishman · 2021-12-08T18:28:54Z

Summary

This adds special handling for ParameterVectorElement instances within
circuits on serialisation to QPY. These needed to be recreated into
the correct class (as opposed to a raw Parameter) so that their sort
order in QuantumCircuit.parameters would remain the same after a QPY
round trip. Without special handling, once there were 10 or more
elements in the vector, the naming (suffixed numbers with no leading
zeros) would cause the sort order to be wrong.

Details and comments

Fix qpy_serialization causes some QuantumCircuit objects to have their parameters re-ordered #7372

On hold til after #7374, because this modifies version 3 of QPY:

rebase over Add QPY serialization for PauliEvolutionGate #7374
merge documentation in QPY
merge qpy compatibility tests
add extra text to existing release note.

This adds special handling for `ParameterVectorElement` instances within circuits on serialisation to QPY. These needed to be recreated into the correct class (as opposed to a raw `Parameter`) so that their sort order in `QuantumCircuit.parameters` would remain the same after a QPY round trip. Without special handling, once there were 10 or more elements in the vector, the naming (suffixed numbers with no leading zeros) would cause the sort order to be wrong. Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

coveralls · 2021-12-08T18:56:35Z

Pull Request Test Coverage Report for Build 1559958820

110 of 119 (92.44%) changed or added relevant lines in 1 file are covered.
19 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.02%) to 83.107%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/qpy_serialization.py	110	119	92.44%

Files with Coverage Reduction	New Missed Lines	%
qiskit/circuit/qpy_serialization.py	19	87.67%

Totals
Change from base Build 1556480050:	-0.02%
Covered Lines:	51854
Relevant Lines:	62394

💛 - Coveralls

qiskit/circuit/qpy_serialization.py

jakelishman · 2021-12-09T09:45:53Z

Caleb found that vector elements used within expressions don't get serialized correctly at the moment:

In [2]: from qiskit import QuantumCircuit
   ...: from qiskit.circuit import ParameterVector
   ...:
   ...: qc = QuantumCircuit(7)
   ...:
   ...: num_features = 14
   ...: entangler_map = [[0, 2], [3, 4], [2, 5], [1, 4], [2, 3], [4, 6]]
   ...:
   ...: entanglement = [[i, i + 1] for i in range(7 - 1)]
   ...: input_params = ParameterVector("x_par", 14)
   ...: user_params = ParameterVector("\u03B8_par", 1)
   ...:
   ...: for i in range(qc.num_qubits):
   ...:     qc.ry(user_params[0], qc.qubits[i])
   ...:
   ...: for source, target in entanglement:
   ...:     qc.cz(qc.qubits[source], qc.qubits[target])
   ...:
   ...: for i in range(qc.num_qubits):
   ...:     qc.rz(-2 * input_params[2 * i + 1], qc.qubits[i])
   ...:     qc.rx(-2 * input_params[2 * i], qc.qubits[i])
   ...:
   ...: from qiskit.circuit.qpy_serialization import dump, load
   ...: import io
   ...: file = io.BytesIO()
   ...: dump(qc, file)
   ...: file.seek(0)
   ...: qpy = load(file)[0]
   ...: qpy.parameters
Out[2]: ParameterView([Parameter(x_par[0]), Parameter(x_par[10]), Parameter(x_par[11]), Parameter(x_par[12]), Parameter(x_par[13]), Parameter(x_par[1]), Parameter(x_par[2]), Parameter(x_par[3]), Parameter(x_par[4]), Parameter(x_par[5]), Parameter(x_par[6]), Parameter(x_par[7]), Parameter(x_par[8]), Parameter(x_par[9]), ParameterVectorElement(θ_par[0])])

mtreinish · 2021-12-09T12:11:14Z

Caleb found that vector elements used within expressions don't get serialized correctly at the moment

I was worried this might be the case, and I should have tested it in my original draft patch file that we used for this PR. I was just hoping things would be smart enough to see a Parameter(name, uuid) would be treated as equivalent to a ParameterVectorElement(name, uuid) but I guess it wasn't. Anyway I've update the PR to distinguish between Parameter and ParameterVectorElement in a ParameterExpression in 2ec1ee4. This did require creating a new versioned payload for parameter expressions though because we need to distinguish the type of the symbols in the symbol map.

test/qpy_compat/test_qpy.py

releasenotes/notes/fix-paramater-vector-qpy-52b16ccefecf8b2e.yaml

qiskit/circuit/qpy_serialization.py

Co-authored-by: Jake Lishman <jakelishman@gmail.com>

…ialized in a qpy file

caleb-johnson · 2021-12-09T16:26:36Z

This PR resolves issues described in #7372 and also handles more complex circuits. Looks good!

test/python/circuit/test_circuit_load_from_qpy.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

Cryoris

LGTM from a functional side. I didn't thoroughly check the code itself as it's kinda hard to understand without comments or docstrings, so adding some might be good for changes we do in the future 🙂

jakelishman

Looks good code-wise, and I like the warnings for incomplete construction a lot. A couple of minor typos (one of which is breaking the docs build).

qiskit/circuit/qpy_serialization.py

jakelishman · 2021-12-09T17:32:53Z

qiskit/circuit/qpy_serialization.py

+PARAMETER_VECTOR_ELEMENT
+------------------------
+
+A PARAMETER_VECTOR_ELEMENT represents a :class:`~qiskit.circuit.ParameterVectorElement`


ParameterVectorElement technically doesn't have any documentation generated, but that's more a bug on its behalf than here. At some point I'd be in favour of us turning on some option in Sphinx to make it an error to write a failed cross-ref, but for now let's just leave it.

qiskit/circuit/qpy_serialization.py

jakelishman · 2021-12-09T17:37:57Z

test/qpy_compat/test_qpy.py

+    return qc
+
+
+def generate_parameter_vector_expression():  # pylint: disable=invalid-name


Perhaps we can disable the upper limit on function name length? It's frustrated me a couple of times too.

Co-authored-by: Jake Lishman <jakelishman@gmail.com>

jakelishman

haha, I just went to approve this and realised that I can't, because technically it's my own PR.

mtreinish

Proxying @jakelishman's approval here: #7381 (review) he opened this PR from my initial WIP patch file while I was working on #7374 so the ownership of this PR got flipped around and github can't really cope with the primary author not being the PR owner.

* Add ParameterVector handling to QPY serialisation This adds special handling for `ParameterVectorElement` instances within circuits on serialisation to QPY. These needed to be recreated into the correct class (as opposed to a raw `Parameter`) so that their sort order in `QuantumCircuit.parameters` would remain the same after a QPY round trip. Without special handling, once there were 10 or more elements in the vector, the naming (suffixed numbers with no leading zeros) would cause the sort order to be wrong. Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Add parameter vector support to PauliEvolutionGate qpy * Add release note * Fix docs typo * Rename PARAMETER_VECTOR -> PARAMETER_VECTOR_ELEMENT * Support ParameterVectorElements in ParameterExpressions * Add compat tests for parameter vector in parameter expression * Fix lint * Update releasenotes/notes/fix-paramater-vector-qpy-52b16ccefecf8b2e.yaml Co-authored-by: Jake Lishman <jakelishman@gmail.com> * Emit warning in deserialization if a parameter vector isn't fully serialized in a qpy file * Serialize a ParameterVectorElement global phase correctly * Update test/python/circuit/test_circuit_load_from_qpy.py Co-authored-by: Julien Gacon <gaconju@gmail.com> * Improve docs * Apply suggestions from code review Co-authored-by: Jake Lishman <jakelishman@gmail.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Julien Gacon <gaconju@gmail.com> (cherry picked from commit 5c4cd2b)

* Add ParameterVector handling to QPY serialisation This adds special handling for `ParameterVectorElement` instances within circuits on serialisation to QPY. These needed to be recreated into the correct class (as opposed to a raw `Parameter`) so that their sort order in `QuantumCircuit.parameters` would remain the same after a QPY round trip. Without special handling, once there were 10 or more elements in the vector, the naming (suffixed numbers with no leading zeros) would cause the sort order to be wrong. Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Add parameter vector support to PauliEvolutionGate qpy * Add release note * Fix docs typo * Rename PARAMETER_VECTOR -> PARAMETER_VECTOR_ELEMENT * Support ParameterVectorElements in ParameterExpressions * Add compat tests for parameter vector in parameter expression * Fix lint * Update releasenotes/notes/fix-paramater-vector-qpy-52b16ccefecf8b2e.yaml Co-authored-by: Jake Lishman <jakelishman@gmail.com> * Emit warning in deserialization if a parameter vector isn't fully serialized in a qpy file * Serialize a ParameterVectorElement global phase correctly * Update test/python/circuit/test_circuit_load_from_qpy.py Co-authored-by: Julien Gacon <gaconju@gmail.com> * Improve docs * Apply suggestions from code review Co-authored-by: Jake Lishman <jakelishman@gmail.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Julien Gacon <gaconju@gmail.com> (cherry picked from commit 5c4cd2b) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

jakelishman requested a review from a team as a code owner December 8, 2021 18:28

jakelishman added on hold Can not fix yet stable backport potential The bug might be minimal and/or import enough to be port to stable labels Dec 8, 2021

jakelishman added this to the 0.19.1 milestone Dec 8, 2021

mtreinish added 2 commits December 8, 2021 16:58

Merge remote-tracking branch 'origin/main' into qpy/parameter-vector

21c6611

Add parameter vector support to PauliEvolutionGate qpy

10b0fb8

mtreinish added Changelog: Bugfix Include in the "Fixed" section of the changelog and removed on hold Can not fix yet labels Dec 8, 2021

mtreinish changed the title ~~Add ParameterVector handling to QPY serialisation~~ Fix ParameterVector handling in QPY serialization Dec 8, 2021

mtreinish added 2 commits December 8, 2021 17:32

Add release note

60c0660

Fix docs typo

a2bfac0

kdk reviewed Dec 8, 2021

View reviewed changes

mtreinish added 2 commits December 9, 2021 06:26

Rename PARAMETER_VECTOR -> PARAMETER_VECTOR_ELEMENT

0974065

Support ParameterVectorElements in ParameterExpressions

2ec1ee4

Merge remote-tracking branch 'origin/main' into qpy/parameter-vector

6de8259

jakelishman commented Dec 9, 2021

View reviewed changes

mtreinish and others added 3 commits December 9, 2021 07:29

Add compat tests for parameter vector in parameter expression

c93e1a7

Fix lint

21bd173

Update releasenotes/notes/fix-paramater-vector-qpy-52b16ccefecf8b2e.yaml

6e7414e

Co-authored-by: Jake Lishman <jakelishman@gmail.com>

mtreinish mentioned this pull request Dec 9, 2021

Store global parameter table for each circuit in QPY Format version 4 #7388

Open

mtreinish added 2 commits December 9, 2021 09:23

Emit warning in deserialization if a parameter vector isn't fully ser…

14fa006

…ialized in a qpy file

Serialize a ParameterVectorElement global phase correctly

d337314

Cryoris reviewed Dec 9, 2021

View reviewed changes

test/python/circuit/test_circuit_load_from_qpy.py Outdated Show resolved Hide resolved

mtreinish and others added 2 commits December 9, 2021 12:01

Update test/python/circuit/test_circuit_load_from_qpy.py

f10e8b2

Co-authored-by: Julien Gacon <gaconju@gmail.com>

Improve docs

cbd45f8

mtreinish added the mod: qpy Related to QPY serialization label Dec 9, 2021

mtreinish requested a review from kdk December 9, 2021 17:13

Cryoris reviewed Dec 9, 2021

View reviewed changes

jakelishman commented Dec 9, 2021

View reviewed changes

Apply suggestions from code review

7315908

Co-authored-by: Jake Lishman <jakelishman@gmail.com>

jakelishman commented Dec 9, 2021

View reviewed changes

mtreinish approved these changes Dec 9, 2021

View reviewed changes

mtreinish added the automerge label Dec 9, 2021

mergify bot merged commit 5c4cd2b into Qiskit:main Dec 9, 2021

mergify bot mentioned this pull request Dec 9, 2021

Fix ParameterVector handling in QPY serialization (backport #7381) #7390

Merged

jakelishman deleted the qpy/parameter-vector branch December 10, 2021 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ParameterVector handling in QPY serialization #7381

Fix ParameterVector handling in QPY serialization #7381

jakelishman commented Dec 8, 2021 •

edited by mtreinish

Loading

coveralls commented Dec 8, 2021 •

edited

Loading

jakelishman commented Dec 9, 2021

mtreinish commented Dec 9, 2021

caleb-johnson commented Dec 9, 2021

Cryoris left a comment

jakelishman left a comment

jakelishman Dec 9, 2021

jakelishman Dec 9, 2021

jakelishman left a comment

mtreinish left a comment

		return qc


		def generate_parameter_vector_expression(): # pylint: disable=invalid-name

Fix ParameterVector handling in QPY serialization #7381

Fix ParameterVector handling in QPY serialization #7381

Conversation

jakelishman commented Dec 8, 2021 • edited by mtreinish Loading

Summary

Details and comments

coveralls commented Dec 8, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1559958820

💛 - Coveralls

jakelishman commented Dec 9, 2021

mtreinish commented Dec 9, 2021

caleb-johnson commented Dec 9, 2021

Cryoris left a comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Dec 9, 2021

Choose a reason for hiding this comment

jakelishman Dec 9, 2021

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

jakelishman commented Dec 8, 2021 •

edited by mtreinish

Loading

coveralls commented Dec 8, 2021 •

edited

Loading