Support `Expr` that Terra 0.24 newly introduced #1915

hhorii · 2023-08-24T13:13:53Z

Summary

Add support of simulation of classical expressions in Terra 0.24

Details and comments

This PR adds experimental feature to support classical expression introduced in Terra 0.24.

The supported expression is for test expression in switch, while and if else. Expr class and its sub-classes are converted newly defined C++ classes in operations.hpp, which are bound to python. AerCompiler supports conversion from branch conditions in switch while, and if else into Expr and then assembles the circuit to AerCircuit with newly introduced C++ classes for Expr.

* Implemented ecr for stabilizer simulator. * Implemented ecr for stabilizer simulator. * Adapted to coding style. * fixed testcode for ecr in stabilizer.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

jakelishman

I haven't had and likely won't have time to review in full depth - I think we're probably going to need to move to a model where that's more the responsibility within the Aer team, but I've just run through quickly and made a couple of notes on the high-level implementation.

In C++ land, I was vaguely thinking that a more efficient simulator/interpreter would want to convert the tree representation of Qiskit's Expr into a linear executable bytecode rather than retaining the tree structure into C++ land (better cache locality of the data, no recursion needed during execution, potential for JIT optimisations while writing out the bytecode, etc). I'm not suggesting you change to that in this PR if you don't think it's necessary, I'm just mentioning it as the path I'd thought made a little more sense for a simulator.

jakelishman · 2023-09-12T13:30:28Z

qiskit_aer/backends/aer_compiler.py

+    def _convert_jump_conditional(self, cond_tuple, bit_map):
        """Convert a condition tuple according to the wire map."""
-        if isinstance(cond_tuple[0], Clbit):
+        if isinstance(cond_tuple, Expr):
+            return cond_tuple
+        elif isinstance(cond_tuple[0], Clbit):
            return (bit_map[cond_tuple[0]], cond_tuple[1])
-        # ClassicalRegister conditions should already be in the outer circuit.
-        return cond_tuple
+        elif isinstance(cond_tuple[0], ClassicalRegister):
+            # ClassicalRegister conditions should already be in the outer circuit.
+            return cond_tuple
+
+        # Classical Expression
+        def _convert_clbit_index(expr):
+            if isinstance(expr, Unary):
+                _convert_clbit_index(expr.operand)
+            elif isinstance(expr, Binary):
+                _convert_clbit_index(expr.left)
+                _convert_clbit_index(expr.right)
+            elif isinstance(expr, Var):
+                if isinstance(expr.var, Clbit):
+                    expr.var = bit_map[expr.var]
+                else:
+                    expr.var = [bit_map[clbit] for clbit in expr.var]
+            return expr
+
+        return (_convert_clbit_index(deepcopy(cond_tuple[0])), cond_tuple[1])


I'm not clear what this function is doing - it appears to me that the if/elif/elif covers all valid .condition fields already, so I don't know what the _convert_clbit_index function is for.

For writing that type of function, you might want to look at the tools in Qiskit that are designed to work more efficiently / easily with the Expr tree, rather than sequences of if/elif statements. The most general form in ExprVisitor, but there are a few pre-defined helpers that do more of the work as well. The full docs are here: https://qiskit.org/documentation/apidoc/circuit_classical.html#working-with-the-expression-tree.

I created _convert_clbit_index() to convert cbit indices with bit_map in the given Expr. Do you mean that this conversion is not necessary?

Definitely I should use ExprVisitor.

It's more that I can't immediately see how control could pass to the definition of that function - I think the if/elif/elif above it is exhaustive.

qiskit_aer/backends/aer_compiler.py

jakelishman · 2023-09-12T13:50:43Z

qiskit_aer/backends/aer_compiler.py

+def _assemble_expr(circ, expr):
+    aer_expr = None
+    if expr is None:
+        aer_expr = None
+    elif isinstance(expr, Value):
+        if isinstance(expr.type, Uint):
+            aer_expr = AerUintValue(expr.type.width, expr.value)
+        elif isinstance(expr.type, Bool):
+            aer_expr = AerBoolValue(expr.value)
+        else:
+            raise AerError(f"invalid value type is specified: {expr.type.__class__}")
+    elif isinstance(expr, Var):
+        aer_expr = AerVar(_assemble_type(expr.type), _assemble_clbit_indices(circ, expr.var))
+    elif isinstance(expr, Cast):
+        aer_expr = AerCast(_assemble_type(expr.type), _assemble_expr(circ, expr.operand))
+    elif isinstance(expr, Unary):
+        aer_expr = AerUnaryExpr(
+            _assemble_unary_operator(expr.op), _assemble_expr(circ, expr.operand)
+        )
+    elif isinstance(expr, Binary):
+        aer_expr = AerBinaryExpr(
+            _assemble_binary_operator(expr.op),
+            _assemble_expr(circ, expr.left),
+            _assemble_expr(circ, expr.right),
+        )
+    else:
+        raise AerError(f"unknown expression: {expr.__class__}")


For example, this is another case where using the ExprVisitor pattern might be a bit more reliable. It looks something like:

class _AssembleExprImpl(ExprVisitor): def __init__(self, circuit): self.circuit = circuit def visit_value(self, node): return AerUintValue(node.type.width, node.value) if isinstance(expr.type, Uint) else AerBoolValue(node.value) def visit_var(self, node): return AerVar(_assemble_type(node.type), _assemble_clbit_indices(self.circuit, node.var)) def visit_cast(self, node): return AerCast(_assemble_type(node.type), self.visit(node.operand)) # ... and so on def _assemble_expr(circ, expr): # You could also just create one `_AssembleExprImpl` per `circuit` in the root assmebler return _AssembleExprImpl(circ).visit(expr)

It's not a huge difference, but it does effectively turn the if/elif/elif chain with many isinstance calls into simple double-dispatch which is exactly two vtable lookups.

Yes. ExprVisitor looks much better than the current my impl.

@jakelishman I updated this PR to use ExprVisitor. I think this class does not have visit() so I created it. Maybe we can use accept() instead of creating this new method.

Yeah ExprVisitor doesn't have a visit method because you call it as expr.accept(visitor); the Expr subclasses handle choosing which method to call, rather than the ExprVisitor doing magic inference.

jakelishman · 2023-09-12T13:56:25Z

qiskit_aer/backends/aer_compiler.py

+        target_clbits = []
+        if isinstance(instruction.operation.target, Clbit):
+            target_clbits = [bit_map[instruction.operation.target]]
+        elif isinstance(instruction.operation.target, Expr):
+            self._list_clbit_from_expr(bit_map, target_clbits, instruction.operation.target)
+        else:
+            target_clbits = [bit_map[c] for c in instruction.operation.target]
+        mark_cargs = set(cargs + target_clbits) - set(instruction.clbits)



Just as a warning: the next version of Qiskit will (very likely) introduce Var fields that are not backed by Clbit instances. This PR is fine for now, but I'll send you the design doc just as soon as I've finished writing it.

Here is the promised design document: Qiskit/RFCs#50

I believe that backward compatibility needs to be maintained regarding Var instantiation that takes Clbit or ClassicalRegister. So, in the new design, there are three types of Var:

based on Clbit

based on ClassicalRegister

based on specific type

In simulation, Var needs to be assigned a specific memory area. I will use the existing memory management in Aer and implicitly create ClassicalRegister for the third type of Vars before simulation.

doichanj · 2023-09-14T08:19:15Z

src/framework/operations.hpp

@@ -287,7 +610,8 @@ struct Op {
  bool conditional = false; // is gate conditional gate
  uint_t conditional_reg; // (opt) the (single) register location to look up for
                          // conditional
-  RegComparison bfunc;    // (opt) boolean function relation
+  BinaryOp binary_op;     // (opt) boolean function relation


Op.bfunc is replaced with Op.binary_op but OpTyp::bfunc and ClassicalRegister::apply_bfunc are still using name bfunc. Is it OK?

Thanks. It should be binary_op.

Hmmm, we still use Operations::OpType::bfunc and keeping apply_bfunc is reasonable.

…er_compiler

jakelishman

I've looked over the Python-space components again, and they look generally fine to me. I can't do the C++ components, sorry - I don't have the time to be getting that deep into Aer any more, but given that the tests pass, I'm guessing that they work ok.

I left a couple of small suggestions for tidying things up using some additional Terra helper methods that already exist / potentially re-using code, but it's not critical; it could be fixed later if necessary.

jakelishman · 2023-10-10T23:05:11Z

qiskit_aer/backends/aer_compiler.py

+    class _ClbitConverter(ExprVisitor):
+        """Apply cbit index conversion in Expr tree"""
+
+        def __init__(self, bit_map):
+            self.bit_map = bit_map
+
+        def visit_value(self, node, /):
+            # pylint: disable=unused-variable
+            return node
+
+        def visit_var(self, node, /):
+            if isinstance(node.var, Clbit):
+                node.var = self.bit_map[node.var]
+            else:
+                node.var = [self.bit_map[clbit] for clbit in node.var]
+            return node
+
+        def visit_cast(self, node, /):
+            node.operand.accept(self)
+            return node
+
+        def visit_unary(self, node, /):
+            node.operand.accept(self)
+            return node
+
+        def visit_binary(self, node, /):
+            node.left.accept(self)
+            node.right.accept(self)
+            return node
+
+        def visit_generic(self, node, /):
+            raise AerError(f"unsupported expression is used: {node.__class__}")


It feels like this visitor should construct the corresponding AerExpr tree, rather than deepcopying the Terra one and mutating it? In particular, mutating Var.var is super unsafe, especially because it's being changed to a value of an incorrect type here. (I also want to be a bit sneaky in Terra 1.0 and make expr.Var and expr.Value immutable; they logically always should have been.)

Can you re-use _AssembleExprImpl here? That does the clbit remapping already, I think?

This class is no longer used in the latest code. Now, Only Var instance is copied.

jakelishman · 2023-10-10T23:06:57Z

qiskit_aer/backends/aer_compiler.py

+        if isinstance(cond_tuple, Expr):
+            return cond_tuple
+        elif isinstance(cond_tuple[0], Clbit):
            return (bit_map[cond_tuple[0]], cond_tuple[1])
-        # ClassicalRegister conditions should already be in the outer circuit.
-        return cond_tuple
+        elif isinstance(cond_tuple[0], ClassicalRegister):
+            # ClassicalRegister conditions should already be in the outer circuit.
+            return cond_tuple
+
+        # left and right expression
+        def _convert_clbit_index(expr):
+            if isinstance(expr, Unary):
+                _convert_clbit_index(expr.operand)
+            elif isinstance(expr, Binary):
+                _convert_clbit_index(expr.left)
+                _convert_clbit_index(expr.right)
+            elif isinstance(expr, Var):
+                if isinstance(expr.var, Clbit):
+                    expr.var = bit_map[expr.var]
+                else:
+                    expr.var = [bit_map[clbit] for clbit in expr.var]
+            return expr
+
+        left = deepcopy(cond_tuple[0]).accept(AerCompiler._ClbitConverter(bit_map))
+        right = cond_tuple[1]
+        return (left, right)


I have a previous review comment about this up top, but in what situations do we reach line 260? As far as I know, a condition can only be an Expr or a tuple[Clbit | ClassicalRegister, int], so the if statements on lines 253 to 239 should be exhaustive, right? In what situations might we get through them?

Thanks. Var is possible to come here. I rewrite this part.

jakelishman · 2023-10-10T23:09:43Z

qiskit_aer/backends/aer_compiler.py

+    def _list_clbit_from_expr(self, bit_map, clbits, expr):
+        if isinstance(expr, Unary):
+            self._list_clbit_from_expr(bit_map, clbits, expr.operand)
+        elif isinstance(expr, Binary):
+            self._list_clbit_from_expr(bit_map, clbits, expr.left)
+            self._list_clbit_from_expr(bit_map, clbits, expr.right)
+        elif isinstance(expr, Var):
+            if isinstance(expr.var, Clbit):
+                clbits += bit_map[expr.var]
+            else:
+                clbits += [bit_map[clbit] for clbit in expr.var]


There's a helper in Terra for iterating only over the Var instances: qiskit.circuit.classical.expr.iter_vars. You could implement this like

from qiskit.circuit.classical import expr def _clbits_from_expr(self, bit_map, node): out = set() for var in expr.iter_vars(node): if isinstance(var.var, Clbit): out.add(bit_map[var.var]) elif isinstance(var.var, ClassicalRegister): out.update(bit_map[bit] for bit in var.var) else: # not handled return out

and you don't need to manually handle all the possible entries in the tree (for example, your current implementation forgets Cast), and Terra will automatically handle any new Expr subclasses that get added for you.

Thanks. I changed the code as you suggested.

jakelishman · 2023-10-10T23:17:38Z

qiskit_aer/backends/aer_compiler.py

-        aer_circ.multiplexer(qubits, params, conditional_reg, label if label else name)
+        aer_circ.multiplexer(qubits, params, conditional_reg, aer_cond_expr,
+                             label if label else name)


I'm surprised that this passes the lint CI, because this line splitting isn't compatible with black, which I thought Aer was using.

releasenotes/notes/support_classical_expr-dd621e5c0fd23a15.yaml

Co-authored-by: Jake Lishman <[email protected]>

doichanj

I think Expr implementation looks fine and I confirm there is no side effect by this change

* initial commit for classical expressions * add classical expression in C++ and its binding * Implemented ecr for stabilizer simulator. (Qiskit#1892) * Implemented ecr for stabilizer simulator. * Implemented ecr for stabilizer simulator. * Adapted to coding style. * fixed testcode for ecr in stabilizer. * remove deploy documentation to /documentation/aer (Qiskit#1891) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * support classical expression evaluation for branches * fix lint errors * change Expr to CExpr * resolve lint errors * suppprt expr for batched shot GPU * use ExprVisitor in AerCompiler to traverse nodes in Expr * fix lint issues * use accept method instead of custom visit method for ExprVisitor in aer_compiler * fix lint errors * refactor aer_compiler * Update releasenotes/notes/support_classical_expr-dd621e5c0fd23a15.yaml Co-authored-by: Jake Lishman <[email protected]> * replace expr rather than modiyfing expr.var * fix lint error --------- Co-authored-by: MarcMaussner <[email protected]> Co-authored-by: Luciano Bello <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Jun Doi <[email protected]> Co-authored-by: Jake Lishman <[email protected]>

hhorii and others added 6 commits August 21, 2023 14:21

initial commit for classical expressions

cc99cf9

add classical expression in C++ and its binding

a208bae

Implemented ecr for stabilizer simulator. (Qiskit#1892)

8b1f843

* Implemented ecr for stabilizer simulator. * Implemented ecr for stabilizer simulator. * Adapted to coding style. * fixed testcode for ecr in stabilizer.

remove deploy documentation to /documentation/aer (Qiskit#1891)

7a74cdd

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

support classical expression evaluation for branches

787e16e

Merge remote-tracking branch 'upstream/main' into support-expr

67cae42

hhorii marked this pull request as ready for review August 29, 2023 05:22

hhorii force-pushed the support-expr branch 3 times, most recently from d7bdb00 to 591c907 Compare August 31, 2023 09:04

fix lint errors

722c9c2

hhorii force-pushed the support-expr branch from 591c907 to 722c9c2 Compare September 1, 2023 00:52

doichanj self-requested a review September 1, 2023 05:18

doichanj added this to the Aer 0.13.0 milestone Sep 1, 2023

doichanj added the Changelog: New Feature Include in the Added section of the changelog label Sep 1, 2023

change Expr to CExpr

5cb5c0f

t-imamichi mentioned this pull request Sep 11, 2023

Why is expr.Expr not compatible with circuit execution? Qiskit/qiskit#10803

Closed

hhorii added 2 commits September 12, 2023 04:52

resolve lint errors

3dff8b5

Merge remote-tracking branch 'upstream/main' into support-expr

1aa991c

jakelishman reviewed Sep 12, 2023

View reviewed changes

doichanj reviewed Sep 14, 2023

View reviewed changes

doichanj and others added 4 commits September 15, 2023 15:35

suppprt expr for batched shot GPU

2042d1a

use ExprVisitor in AerCompiler to traverse nodes in Expr

97621ef

fix lint issues

96fc240

use accept method instead of custom visit method for ExprVisitor in a…

1b63288

…er_compiler

hhorii force-pushed the support-expr branch 2 times, most recently from a99d63b to 245b4fa Compare September 27, 2023 06:20

fix lint errors

7362754

hhorii force-pushed the support-expr branch from 6c07c1b to 7362754 Compare September 27, 2023 08:50

doichanj previously approved these changes Oct 2, 2023

View reviewed changes

hhorii and others added 3 commits October 2, 2023 11:02

Merge branch 'main' into support-expr

8021369

Merge branch 'main' into support-expr

9b9e2bf

Merge remote-tracking branch 'upstream/main' into support-expr

134693e

hhorii dismissed doichanj’s stale review via 134693e October 6, 2023 03:01

doichanj previously approved these changes Oct 6, 2023

View reviewed changes

doichanj mentioned this pull request Oct 6, 2023

Release 0.13.0 #1953

Closed

hhorii and others added 2 commits October 10, 2023 12:19

Merge branch 'main' into support-expr

24ebcb5

Merge remote-tracking branch 'upstream/main' into support-expr

071ae43

hhorii dismissed doichanj’s stale review via 071ae43 October 10, 2023 14:22

jakelishman reviewed Oct 10, 2023

View reviewed changes

hhorii and others added 4 commits October 11, 2023 18:20

refactor aer_compiler

a93c5fa

Update releasenotes/notes/support_classical_expr-dd621e5c0fd23a15.yaml

4fdaae6

Co-authored-by: Jake Lishman <[email protected]>

replace expr rather than modiyfing expr.var

5c62d3f

fix lint error

cc93d63

hhorii force-pushed the support-expr branch from 7299826 to cc93d63 Compare October 11, 2023 10:08

doichanj approved these changes Oct 13, 2023

View reviewed changes

doichanj merged commit e434c59 into Qiskit:main Oct 13, 2023
31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `Expr` that Terra 0.24 newly introduced #1915

Support `Expr` that Terra 0.24 newly introduced #1915

hhorii commented Aug 24, 2023 •

edited

Loading

jakelishman left a comment

jakelishman Sep 12, 2023

hhorii Sep 22, 2023 •

edited

Loading

jakelishman Sep 22, 2023

jakelishman Sep 12, 2023

hhorii Sep 22, 2023

hhorii Sep 26, 2023

jakelishman Sep 26, 2023

jakelishman Sep 12, 2023

jakelishman Sep 14, 2023

hhorii Sep 22, 2023

doichanj Sep 14, 2023 •

edited

Loading

hhorii Sep 22, 2023

hhorii Sep 26, 2023

jakelishman left a comment

jakelishman Oct 10, 2023

hhorii Oct 11, 2023

jakelishman Oct 10, 2023

hhorii Oct 11, 2023

jakelishman Oct 10, 2023

hhorii Oct 11, 2023

jakelishman Oct 10, 2023

doichanj left a comment

Support Expr that Terra 0.24 newly introduced #1915

Support Expr that Terra 0.24 newly introduced #1915

Conversation

hhorii commented Aug 24, 2023 • edited Loading

Summary

Details and comments

jakelishman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhorii Sep 22, 2023 • edited Loading

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

doichanj Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman 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

doichanj left a comment

Choose a reason for hiding this comment

Support `Expr` that Terra 0.24 newly introduced #1915

Support `Expr` that Terra 0.24 newly introduced #1915

hhorii commented Aug 24, 2023 •

edited

Loading

hhorii Sep 22, 2023 •

edited

Loading

doichanj Sep 14, 2023 •

edited

Loading