Add `Expr` support to QPY for conditions and targets #10392

jakelishman · 2023-07-06T02:25:06Z

Summary

This adds support to QPY for the current Expr nodes, for all instruction parameters and conditions. The Expr tree is written out to the file in a sort of forwards Polish notation; each node has a type code and header, followed by a type-code-specific number of Expr children.

While only IfElseOp.condition, WhileLoopOp.condition and SwitchCaseOp.target are allowed to have these nodes in Terra's data model right now, the QPY serialisation does not need to have this arbitrary restriction, and it's much easier just to write the general case.

The backwards-compatibility guarantees of QPY are now brought to bear on the Unary.Op and Binary.Op enumeration values. They were already marked in the source code as their values needing to be part of the stable public interface, and their use in things like QPY is the reason why.

Details and comments

Close #10289. Depends on #10359. Changelog as part of #10331.

qiskit-bot · 2023-07-06T02:25:09Z

One or more of the the following people are requested to review this:

@Eric-Arellano
@HuangJunye
@javabster
@Qiskit/terra-core
@mtreinish
@nkanazawa1989

coveralls · 2023-07-06T03:00:09Z

Pull Request Test Coverage Report for Build 5603129027

202 of 241 (83.82%) changed or added relevant lines in 5 files are covered.
92 unchanged lines in 7 files lost coverage.
Overall coverage increased (+0.02%) to 86.076%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/qpy/binary_io/circuits.py	22	23	95.65%
qiskit/qpy/binary_io/value.py	102	120	85.0%
qiskit/qpy/type_keys.py	45	65	69.23%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/instruction_durations.py	1	92.96%
qiskit/transpiler/passmanager_config.py	1	97.26%
crates/qasm2/src/lex.rs	2	91.65%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%
qiskit/providers/fake_provider/fake_backend.py	4	91.12%
qiskit/compiler/transpiler.py	13	92.89%
qiskit/circuit/quantumcircuit.py	69	94.18%

Totals
Change from base Build 5597428209:	0.02%
Covered Lines:	72804
Relevant Lines:	84581

💛 - Coveralls

This adds support to QPY for the current `Expr` nodes, for *all* instruction parameters and conditions. The `Expr` tree is written out to the file in a sort of forwards Polish notation; each node has a type code and header, followed by a type-code-specific number of `Expr` children. While only `IfElseOp.condition`, `WhileLoopOp.condition` and `SwitchCaseOp.target` are allowed to have these nodes in Terra's data model right now, the QPY serialisation does not need to have this arbitrary restriction, and it's much easier just to write the general case. The backwards-compatibility guarantees of QPY are now brought to bear on the `Unary.Op` and `Binary.Op` enumeration values. They were already marked in the source code as their values needing to be part of the stable public interface, and their use in things like QPY is the reason why.

jakelishman · 2023-07-19T11:27:05Z

Now rebased over main.

mtreinish

Overall this LGTM, thanks for working through this. Using the expression visitor class works quite naturally for this which makes it easier to deal with. I just left a few small comments and questions inline, most are my idle musings and not really critical.

mtreinish · 2023-07-19T18:19:31Z

test/qpy_compat/test_qpy.py

@@ -632,8 +715,10 @@ def generate_circuits(version_parts):
    if version_parts >= (0, 24, 1):
        output_circuits["open_controlled_gates.qpy"] = generate_open_controlled_gates()
        output_circuits["controlled_gates.qpy"] = generate_controlled_gates()
-    if version_parts > (0, 24, 2):
+    if version_parts >= (0, 24, 2):


Oh, good catch yeah now that 0.24.2 has been released this was skipping the layout test from 0.24.2 -> 0.25.0

oh, I think I may have done that accidentally - it was part of a merge conflict haha

mtreinish · 2023-07-19T18:22:39Z

qiskit/qpy/formats.py

        "condition_register_size",
        "condition_value",
        "num_ctrl_qubits",
        "ctrl_state",
    ],
 )
-CIRCUIT_INSTRUCTION_V2_PACK = "!HHHII?HqII"
+CIRCUIT_INSTRUCTION_V2_PACK = "!HHHIIBHqII"


I assume you didn't version this pack string because ? and B are both a single byte and are interchangeable so nothing will break going from the old format to the new (except the parsing but it is a new format so that's fine).

Ah, yep I see that called out in the docs too so yeah that makes sense.

yeah, exactly - and the struct-pack conversion to ? guarantees that the written value is exactly 0 or 1, so there's precise ABI compatibility

mtreinish · 2023-07-19T18:33:03Z

qiskit/qpy/binary_io/value.py

@@ -123,9 +227,9 @@ def _read_parameter_expression(file_obj):
    if _optional.HAS_SYMENGINE:
        import symengine

-        expr = symengine.sympify(parse_expr(file_obj.read(data.expr_size).decode(common.ENCODE)))
+        expr_ = symengine.sympify(parse_expr(file_obj.read(data.expr_size).decode(common.ENCODE)))


Heh, I didn't expect to ever have a conflict with this string. I should have written out symbolic_expression here or something

Eh, I probably should have thought that it's probably a relatively common abbreviation before squatting the expr name for the module I added.

qiskit/qpy/__init__.py

mtreinish · 2023-07-19T18:43:47Z

qiskit/qpy/binary_io/value.py

+                # decode fine so it's not worth another special case.  They'll encode to
+                # b"\xff\x80\x00\x00...", but we could encode them to b"\x80\x00\x00...".
+                num_bytes = (node.value.bit_length() // 8) + 1
+                buffer = node.value.to_bytes(num_bytes, "big", signed=True)


Heh, I would have totally missed setting this explicitly. At least it looks like it defaults to big endian and we'd have matched the endianness for qpy.

I guess this is needed in case there is a >63 bit register being used in the expression so we can't use a fixed width integer encoding here.

Yeah, that's the exact reason I put this conversion in. int.to_bytes and int.from_bytes both require you to set the endianness, so I had to remember lol.

qiskit/qpy/binary_io/value.py

mtreinish

LGTM, thanks for the fast update.

* Add `Expr` support to QPY for conditions and targets This adds support to QPY for the current `Expr` nodes, for *all* instruction parameters and conditions. The `Expr` tree is written out to the file in a sort of forwards Polish notation; each node has a type code and header, followed by a type-code-specific number of `Expr` children. While only `IfElseOp.condition`, `WhileLoopOp.condition` and `SwitchCaseOp.target` are allowed to have these nodes in Terra's data model right now, the QPY serialisation does not need to have this arbitrary restriction, and it's much easier just to write the general case. The backwards-compatibility guarantees of QPY are now brought to bear on the `Unary.Op` and `Binary.Op` enumeration values. They were already marked in the source code as their values needing to be part of the stable public interface, and their use in things like QPY is the reason why. * Improve documentation of `EXPRESSION` payload * Factor out magic numbers from discriminator sizes

jakelishman added Changelog: New Feature Include in the "Added" section of the changelog mod: qpy Related to QPY serialization labels Jul 6, 2023

jakelishman added this to the 0.25.0 milestone Jul 6, 2023

jakelishman requested a review from a team as a code owner July 6, 2023 02:25

jakelishman force-pushed the expr/qpy branch from a27c4c9 to b0c56bd Compare July 6, 2023 10:39

jakelishman mentioned this pull request Jul 7, 2023

Add support for QuantumCircuit.layout to qpy #10148

Merged

4 tasks

mtreinish self-assigned this Jul 13, 2023

jakelishman mentioned this pull request Jul 17, 2023

Add support for semantic equality checking of Exprs in DAGCircuit #10367

Merged

kdk mentioned this pull request Jul 18, 2023

Implementation of classical expressions: RFC 10 #10239

Closed

kdk added the on hold Can not fix yet label Jul 18, 2023

jakelishman force-pushed the expr/qpy branch from b0c56bd to 39fbb39 Compare July 19, 2023 11:26

jakelishman removed the on hold Can not fix yet label Jul 19, 2023

mtreinish reviewed Jul 19, 2023

View reviewed changes

jakelishman added 2 commits July 19, 2023 20:06

Improve documentation of EXPRESSION payload

912c1e3

Factor out magic numbers from discriminator sizes

11e2eaa

mtreinish approved these changes Jul 19, 2023

View reviewed changes

mtreinish enabled auto-merge July 19, 2023 19:18

mtreinish added this pull request to the merge queue Jul 19, 2023

mtreinish mentioned this pull request Jul 19, 2023

Add in qpy the support for Discriminator and Kernel #10327

Merged

Merged via the queue into Qiskit:main with commit 02886d0 Jul 20, 2023

jakelishman deleted the expr/qpy branch July 20, 2023 08:16

kt474 mentioned this pull request Jul 28, 2023

QPY Terra 0.25 updates Qiskit/qiskit-ibm-provider#684

Merged

jakelishman mentioned this pull request Jul 31, 2023

QPY invalid payload generation via compose() or QuantumCircuit.control() #9746

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Expr` support to QPY for conditions and targets #10392

Add `Expr` support to QPY for conditions and targets #10392

jakelishman commented Jul 6, 2023

qiskit-bot commented Jul 6, 2023

coveralls commented Jul 6, 2023 •

edited

Loading

jakelishman commented Jul 19, 2023

mtreinish left a comment

mtreinish Jul 19, 2023

jakelishman Jul 19, 2023

mtreinish Jul 19, 2023

mtreinish Jul 19, 2023

jakelishman Jul 19, 2023

mtreinish Jul 19, 2023

jakelishman Jul 19, 2023

mtreinish Jul 19, 2023

jakelishman Jul 19, 2023

mtreinish left a comment

Add Expr support to QPY for conditions and targets #10392

Add Expr support to QPY for conditions and targets #10392

Conversation

jakelishman commented Jul 6, 2023

Summary

Details and comments

qiskit-bot commented Jul 6, 2023

coveralls commented Jul 6, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5603129027

💛 - Coveralls

jakelishman commented Jul 19, 2023

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

Add `Expr` support to QPY for conditions and targets #10392

Add `Expr` support to QPY for conditions and targets #10392

coveralls commented Jul 6, 2023 •

edited

Loading