QuantumCircuit and DAGCircuit updates for optional registers. #5498

kdk · 2020-12-09T22:38:52Z

Summary

Following #5486 , updates DAGCircuit and QuantumCircuit classes to support optional registers, and to prefer inspecting bits over registers where possible.

Details and comments

~~on-hold until #5486 merges.~~

Commits for just this PR (without #5486 ): https://github.com/Qiskit/qiskit-terra/pull/5498/files/45ba1d598141b90818c53abaadf1d64a80102e8b..d13ef0e35946d20facf569f2eadd65a93f678174

mtreinish

This LGTM, just a few comments and questions inline

qiskit/dagcircuit/dagcircuit.py

mtreinish · 2021-02-22T16:25:00Z

qiskit/dagcircuit/dagcircuit.py

@@ -310,8 +314,7 @@ def _add_wire(self, wire):
            self.output_map[wire] = outp_node
            self._multi_graph.add_edge(inp_node._node_id,
                                       outp_node._node_id,
-                                       {'name': wire_name,
-                                        'wire': wire})
+                                       {'wire': wire})


Do you think it would make sense to drop the dict wrapper here and just attach the bit/wire on the edge directly? We'd probably have to change some access patters from edge['wire'] to edge not sure how much that is used.

That's a good idea, it seems like only a handful of places check this property, so it seems a good time to remove the unneeded dict.

Added in 664485e .

After starting Qiskit/rustworkx#252 I expect it was originally a dict because networkx only lets you weight edges with a dict of attributes instead of an object. After we switched to retworkx there wasn't really a reason to change it until now.

mtreinish · 2021-02-22T16:32:18Z

qiskit/dagcircuit/dagcircuit.py

+                                              for creg in target_cregs
+                                              if wire_map[bit] in creg)
+                    except StopIteration:
+                        raise DAGCircuitError('Did not find creg containing '


It might be worth updating the docstring to add this error case under the DAGCircuitError raise section. Something like, "also when the specified condition is not present in a classical register"

Added in 686ed2c .

mtreinish · 2021-02-22T16:34:40Z

qiskit/dagcircuit/dagcircuit.py

                        # Raise if wire_map maps condition creg on to more than one
                        # creg in target DAG.
                        raise DAGCircuitError('wire_map maps conditional '
                                              'register onto more than one creg.')

-                    if 2**(bit.index) & cond_val:
-                        new_cond_val += 2**(wire_map[bit].index)
+                    if 2**(cond_creg[:].index(bit)) & cond_val:


Is the [:] just there to workaround around a Register not being a list and not having an index method?

That's right. (Alternately, we could consider building the bit-> index dict like we do elsewhere.)

qiskit/dagcircuit/dagcircuit.py

mtreinish · 2021-02-22T16:40:49Z

qiskit/dagcircuit/dagdependency.py

+    carg1 = [node1.cargs[i] for i in range(0, len(node1.cargs))]
+    carg2 = [node2.cargs[i] for i in range(0, len(node2.cargs))]


Wow, that's surprising nobody caught that we were putting qubits there until now.

It seems like you'd hit this only if you were looking to run TemplateOptimization on a circuit with more than one qreg (but if you did, you'd hit it right away). Might be an edge case no one had run into yet, since its fairly new, and not in the preset pass managers.

mtreinish · 2021-02-22T16:44:14Z

qiskit/dagcircuit/dagnode.py

@@ -102,27 +102,41 @@ def __str__(self):
        return str(id(self))

    @staticmethod
-    def semantic_eq(node1, node2):
+    def semantic_eq(node1, node2, bit_indices1, bit_indices2):


I'm a bit concerned about this change from a backwards compat and interop PoV. Like someone could have been using semantic eq to compare dag nodes outside of DAGCircuit.__eq__ before right and this will now break them? Maybe making them optional and just having a fallback to node1_qargs = node1._qargs if it's not specified.

That makes sense. I'll add that (with a deprecation warning).

Added in 0dc3d7a .

mtreinish · 2021-02-22T16:46:33Z

releasenotes/notes/remove-name-attribute-from-dagcircuit-wires-b54cd6abfce95da4.yaml

@@ -0,0 +1,6 @@
+---
+other:


I was thinking this might be an upgrade note instead, but I guess it is kind of an edge case. It's unlikely that people are depending on the name field on the edge dict of a DAGCircuit

Sure, moved in 664485e .

mtreinish · 2021-02-22T20:44:13Z

The other thing I forgot to ask in the earlier comments was what DAGDependency, there aren't any other changes needed there? It doesn't use wires on the graph like DAGCircuit does, but I thought it tracked the wires in the object (and per node)

kdk · 2021-02-22T23:03:45Z

The other thing I forgot to ask in the earlier comments was what DAGDependency, there aren't any other changes needed there? It doesn't use wires on the graph like DAGCircuit does, but I thought it tracked the wires in the object (and per node)

Appears not to be, though I'm not the most familiar with the code here. The only references I found to the wires/qargs was in add_op_node and _does_commute, but its possible I missed cases. (Also, maybe DAGDependency should be updated to have add_*bits methods added as was done for DAGCircuit in #5486 .)

Updates DAGCircuit._check_edgemap_registers: - Removes unused valregs and valreg arguments and associated code. - Renames edge_map and keyregs arguments to inbound_wires and inbound_regs. Removes need for proxy register map. - Renames several variables to match their outer context in DAGCircuit.substitute_node_with_dag.

These are redundant of preceding self.qregs updates, which already update self._qubits via the BlueprintCircuit.qregs setter.

…ters.

mtreinish

LGTM< thanks for updating it. I think this is ready to go now.

mtreinish · 2021-02-24T13:01:24Z

qiskit/dagcircuit/dagcircuit.py

+                raise DAGCircuitError("(qu)bit %s not found in %s" %
+                                      (wire, amap))


If this error ever got raised to the user it would make no sense. Not that it made much sense before. Being an internal function it should hopefully only come up during development and not be a big deal.

…internals

kdk added on hold Can not fix yet optional-registers labels Dec 9, 2020

kdk added this to the 0.17 milestone Dec 9, 2020

kdk requested review from maddy-tod, nkanazawa1989, nonhermitian and a team as code owners December 9, 2020 22:38

kdk mentioned this pull request Dec 10, 2020

Converters, quantum_info, assembler, scheduler and circuit library updates for optional registers. #5504

Merged

kdk force-pushed the quantumcircuit-optional-registers-circuit-internals branch 2 times, most recently from 060ffbb to d13ef0e Compare December 11, 2020 22:55

This was referenced Dec 11, 2020

Transpiler updates for optional registers. #5515

Merged

Visualization updates for optional registers. #5519

Merged

Cleanup unneeded Register use in standard library definitions. #5529

Closed

kdk mentioned this pull request Jan 6, 2021

API changes for optional registers in QuantumCircuit. #5486

Merged

kdk force-pushed the quantumcircuit-optional-registers-circuit-internals branch 2 times, most recently from acc066e to 82897ed Compare January 20, 2021 21:58

kdk force-pushed the quantumcircuit-optional-registers-circuit-internals branch from 82897ed to 2eab8aa Compare February 17, 2021 18:36

mtreinish self-assigned this Feb 17, 2021

kdk force-pushed the quantumcircuit-optional-registers-circuit-internals branch from 2eab8aa to 2e1d399 Compare February 17, 2021 23:58

kdk removed the on hold Can not fix yet label Feb 18, 2021

mtreinish reviewed Feb 22, 2021

View reviewed changes

kdk added 7 commits February 23, 2021 15:25

DAGNode.semantic_eq to use {q,c}arg circuit index for arg equality.

0dc3d7a

Remove "name" attribute from DAGCircuit wires.

664485e

Update commutative cancellation passes to prefer Bits over Registers.

fc96765

Update DAGCircuit._map_condition to find registers of mapped clbits.

686ed2c

Add internal bit set to QuantumCircuit for fast membership checks.

b940c77

Update QuantumCircuit checks to prefer Bits over Registers.

3d7e789

kdk added 10 commits February 23, 2021 15:25

Update DAGCircuit tests to prefer Bits over Registers.

802e27d

Update circuit.add_control to prefer Bits over Registers.

9cecda3

Update QuantumCircuit.qasm to prefer Bits over Registers.

1ee2c8a

Update DAGDependency to prefer Bits over Registers.

0625278

Update compose QASM generation to prefer Bits over Registers.

a1bd0f8

Update QuantumCircuit.depth to prefer inspecting Bits over Registers.

a1427fa

Update QuantumCircuit methods to prefer inspecting Bits over Registers.

e74b88a

Update DAGCircuit methods to prefer inspecting Bits over Registers.

112e156

Remove redundant self._qubits updates from arithmetic circuits.

5948e4d

These are redundant of preceding self.qregs updates, which already update self._qubits via the BlueprintCircuit.qregs setter.

Update test_circuit_registers.py to prefer inspecting Bits over Regis…

3a306da

…ters.

kdk force-pushed the quantumcircuit-optional-registers-circuit-internals branch from 2e1d399 to 3a306da Compare February 23, 2021 20:37

mtreinish approved these changes Feb 24, 2021

View reviewed changes

mtreinish added the automerge label Feb 24, 2021

Merge branch 'master' into quantumcircuit-optional-registers-circuit-…

1f0f0cd

…internals

mergify bot merged commit a026b62 into Qiskit:master Feb 24, 2021

kdk added the Changelog: None Do not include in changelog label Mar 31, 2021

kdk mentioned this pull request Apr 19, 2021

Add @property and setter for DAGNode.name #6199

Merged

kdk mentioned this pull request Oct 19, 2021

Bug/fix reverse bits with isolated qubits #6762

Closed

		carg1 = [node1.cargs[i] for i in range(0, len(node1.cargs))]
		carg2 = [node2.cargs[i] for i in range(0, len(node2.cargs))]

		raise DAGCircuitError("(qu)bit %s not found in %s" %
		(wire, amap))

		@@ -0,0 +1,6 @@
		---
		other:

QuantumCircuit and DAGCircuit updates for optional registers. #5498

QuantumCircuit and DAGCircuit updates for optional registers. #5498

Uh oh!

Conversation

kdk commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtreinish commented Feb 22, 2021

Uh oh!

kdk commented Feb 22, 2021

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kdk commented Dec 9, 2020 •

edited

Loading