Add support for semantic equality checking of Exprs in DAGCircuit
#10367
Conversation
jakelishman
added
the
Changelog: New Feature
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5592058798
💛 - Coveralls |
jakelishman
force-pushed
the
expr/dag-eq
branch
2 times, most recently
from
6d3a266
to
0abfb1f
Compare
This adds support to semantically check the variables of `Expr` nodes that occur within `DAGCircuit` constructs, using similar logic to the remapping done by the existing condition checks. As a knock-on effect of how the control-flow introspection was added to allow these checks, the canonicalisation performed by `qiskit.test._canonical.canonicalize_control_flow` is now inherent to the `DAGCircuit` equality methods, largely removing the need for that explicit canonical form in tests. The dispatch of `QuantumCircuit.__eq__` to `DAGCircuit.__eq__` means that direct comparisons of `QuantumCircuit`s will also benefit from this change. As a partial implementation detail, the semantic checking is achieved and defined by the function `expr.structurally_equivalent`, with key functions for mapping the variables. The naming discrepancy (semantic versus structural) is deliberate. Classical expressions are considered "equal" if and only if their tree structures match exactly; there is no attempt to move the classical expressions into some canonical form, as this is not typically simple; even mathematically symmetric operations typically impact the order of sub-expression evaluation, and we don't want to get into the weeds of that. Better to just define equality as "structurally exactly equal", and have the key function just so alpha-renaming-compatible variables can still be canonicalised into something that can be compared between expressions.
| self.other = other.left | ||
| if not node.left.accept(self): | ||
| return False | ||
| self.other = other.right |
There was a problem hiding this comment.
Ah, clever! You reset the reference using the value of self.other captured in the stack.
There was a problem hiding this comment.
Yeah exactly - when I originally thought about this I thought I was going to need to have the visitor store a stack parameter itself, then I realised that we just get it for free through the recursive call structure. I would have liked to avoid the self.other attribute and just explicitly pass the state, but that would have involved a refactor of the visitor base class, and I'm not certain there was a huge benefit.
qiskit/converters/circuit_to_dag.py
Outdated
| qubit_order (Iterable[Qubit] or None): the ordered that the qubits should be indexed in the | ||
| output DAG. Defaults to the same order as in the circuit. | ||
| clbit_order (Iterable[Clbit] or None): the ordered that the clbits should be indexed in the | ||
| output DAG. Defaults to the same order as in the circuit. |
There was a problem hiding this comment.
ordered => order, in both places
| @@ -16,7 +16,7 @@ | |||
| from qiskit.dagcircuit.dagcircuit import DAGCircuit | |||
|
|
|||
|
|
|||
| def circuit_to_dag(circuit, copy_operations=True): | |||
| def circuit_to_dag(circuit, copy_operations=True, *, qubit_order=None, clbit_order=None): | |||
There was a problem hiding this comment.
As it is, this function could be used to create a DAG that doesn't have the same qubits as circuit, at least in the case where the bits aren't in a register or used by an instruction, right? Perhaps we should instead take iterables of indices to reorder by, or do some checking to make sure all bits from circuit are accounted for, and no extra bits were provided.
There was a problem hiding this comment.
I'm not a huge fan of providing indices into a foreign list because it tends to be annoying to compute and it can be confusing whether it's a forwards or backwards permutation ("put bit i in new position order[i]" or "put bit order[i] in new position i" - both make sense), and we'd still need to check for duplicate indices and the correct length of list anyway. I can put in a check that the bits are an exact permutation, though, no problem.
Co-authored-by: Kevin Hartman <kevin@hart.mn>
Summary
This adds support to semantically check the variables of
Exprnodes that occur withinDAGCircuitconstructs, using similar logic to the remapping done by the existing condition checks. As a knock-on effect of how the control-flow introspection was added to allow these checks, the canonicalisation performed byqiskit.test._canonical.canonicalize_control_flowis now inherent to theDAGCircuitequality methods, largely removing the need for that explicit canonical form in tests.The dispatch of
QuantumCircuit.__eq__toDAGCircuit.__eq__means that direct comparisons ofQuantumCircuits will also benefit from this change.As a partial implementation detail, the semantic checking is achieved and defined by the function
expr.structurally_equivalent, with key functions for mapping the variables. The naming discrepancy (semantic versus structural) is deliberate. Classical expressions are considered "equal" if and only if their tree structures match exactly; there is no attempt to move the classical expressions into some canonical form, as this is not typically simple; even mathematically symmetric operations typically impact the order of sub-expression evaluation, and we don't want to get into the weeds of that. Better to just define equality as "structurally exactly equal", and have the key function just so alpha-renaming-compatible variables can still be canonicalised into something that can be compared between expressions.Details and comments
Close #10359. Depends on #10362 (and the rest of its chain). Changelog to come as part of #10331.