API changes for optional registers in QuantumCircuit. #5486

kdk · 2020-12-08T15:43:38Z

Summary

Updates to Bit, Register, QuantumCircuit and DAGCircuit classes to move from Registers as the core data structure unit toward making Bits fundamental and Registers optional.

Details and comments

This PR covers only the API changes required to create and add Bits to QuantumCircuits, DAGCircuits and Registers. Subsequent PRs:

Avoiding Bit.register and .index internally in QuantumCircuit and DAGCircuit (and thus supporting Registers as optional)
Updates as above, for quantum_info, scheduler, and assembler
Updates as above, for all transpiler passes
Updates as above, for visualization tools
Updates as above, for circuit library
Deprecate Bit.index and Bit.register properties
Deprecate creation of implicit 'q'/'c' registers from QuantumCircuit int constructor

See working branch master...kdk:quantumcircuit-optional-registers .

mtreinish

So I think this looks good and I like the direction. Just a couple of questions below and a few inline comments/questions:

For a register-free bit it basically is just a type/class and a hash/id for the object? Longer term It does make we wonder if we just want to use int indices for bits in QuantumCircuit/DAGCircuit and registers are lists of ints. (ie basically metadata around the bits in a circuit). But that would be a much larger change and probably too hard to make. (this is more just an idle thought, I do think the direction in this PR makes the most sense)
Have you done any performance testing on a registerless circuit vs a register one? I know that we've spent a bunch of time tuning the register <-> bit creation and comparisons as operations with bits end up being a lot of the checks we do in the transpiler. Looking at the new bit classes and how __repr__ is changed I think we should look into this ahead of time.

mtreinish · 2021-01-05T19:56:03Z

qiskit/circuit/bit.py

+        if (register, index) == (None, None):
+            self._register = None
+            self._index = None
+            self._hash = object.__hash__(self)


Is there a difference between this and hash(self)?

hash(self) would call the overridden Bit.__hash__, so this is only here to sidestep and use the default object hash for new-style bits (while allowing backwards compatibility for old-style). I'll add a comment to clarify, if you're okay with this approach.

ah ok, yeah that makes sense. Yeah I think having a comment about this is a good idea. It is a bit confusing.

Comment added in 528f8593f8e2182f9d71303ba11ba8dac44a22b0 .

mtreinish · 2021-01-05T20:12:43Z

qiskit/circuit/bit.py

@@ -49,33 +54,50 @@ def _update_hash(self):
    @property
    def register(self):
        """Get bit's register."""
+        if (self._register, self._index) == (None, None):
+            raise CircuitError('Attmped to query register of a new-style Bit.')


I would probably say registerless bit instead of new-style. I think this context around new-style is temporary.

Agree the context is temporary and only for the transition. I went with old- and new-style over registerless because the new Bits can still be a part of a register, which might be confusing. There are only a few places that call out the differences, so I'm happy to update if you think it's clearer.

It would be clearer to me. In my head this kind of bit is always registerless even if it's part of a register it's still a registerless bit right (the bit doesn't have a register in its context) but instead the register contains the bits? It's splitting hairs but that's how I would think about it. But, I'll defer to others because what's clearer to me isn't necessarily clear for everyone else and this isn't a big deal either way.

Okay, good point. For now, I'll leave these in place and when we're writing documentation/release notes, we can decide how best to phrase it (and update the warnings accordingly).

mtreinish · 2021-01-05T20:36:26Z

test/python/circuit/test_registerless_circuit.py

@@ -43,6 +44,61 @@ def test_circuit_constructor_wires_wrong_mix(self):
        self.assertRaises(CircuitError, QuantumCircuit, 1, ClassicalRegister(2))


+class TestAddingBitsWithoutRegisters(QiskitTestCase):


The only test I would think to add here is one for the mixed register, registerless bit case that index in gates works as expected. But that might be in one of the follow on commits, I haven't looked at those yet.

Good point. I don't think there's a test covering mixed registered and registerless Bits in the tests covering addition of gates on a specific qubit index (updated in #5498). I'll add one there.

Test added in 71ec0a4 .

mtreinish · 2021-01-05T20:40:14Z

qiskit/circuit/bit.py

+        if (self._register, self._index) == (None, None):
+            return "%s(%s)" % (self.__class__.__name__, hex(id(self)))


Is there a reason not to cache this? It definitely will be faster than the repr based on the register case, but we still end up using this a lot (because we use str(qargs) as the sort key for lexicographical_topological_sort()) and this basically gets called once for each bit in an op's qargs list each time we create a new DAGNode. So I wonder what the overhead is going to be for doing the string manipulation and hex conversion once per op vs once per bit.

This is another sidestep to have new-style Bits look more like placeholder objects (though looking now, similarly using object.__str__ would've been better). I'll update this.

For lexicographical_topological_sort(), I was hoping to remove the string comparison because we would only need an object comparison with new-style Bits, but looking again, rx.lexicographical_topological_sort expects a key function that returns a string. I missed this in the DAGCircuit PR so this is a good catch, I'll update DAGNode.sort_key there and if string construction still shows up in profiling, we can resume caching.

Yeah doing the string comparison on the retworkx rust side was done for performance. When we first added that function about a year ago having the callback return a string key was a conscious tradeoff between covering a wider set of use cases and performance for how DAGCircuit used it because at least back then the testing showed rust side string comparison to be faster than a python callback returning a bool, but this might have changed some in the past year).

kdk · 2021-01-06T21:50:29Z

So I think this looks good and I like the direction. Just a couple of questions below and a few inline comments/questions:

1. For a register-free bit it basically is just a type/class and a hash/id for the object? Longer term It does make we wonder if we just want to use int indices for bits in QuantumCircuit/DAGCircuit and registers are lists of ints. (ie basically metadata around the bits in a circuit). But that would be a much larger change and probably too hard to make. (this is more just an idle thought, I do think the direction in this PR makes the most sense)

That's right. Long term, converting to ints is something to consider, but would be a larger change (and might have some design questions around if Bits are implicitly ordered or if their order is governed by the circuit).

2. Have you done any performance testing on a registerless circuit vs a register one? I know that we've spent a bunch of time tuning the register <-> bit creation and comparisons as operations with bits end up being a lot of the checks we do in the transpiler. Looking at the new bit classes and how `__repr__` is changed I think we should look into this ahead of time.

Agree, I've done some preliminary testing on the development branch of the final state (all converted to new-style Bits) but only to check the overall design for large regressions and not covering the intermediate state where we need to check and toggle between old- and new-style (which I would expect to be slower, but not substantially).

mtreinish

LGTM, a couple more quick inline comments and questions. But nothing really blocking (lets just fix the typo in the exception message before merging).

qiskit/circuit/bit.py

mtreinish · 2021-02-11T20:46:54Z

qiskit/circuit/register.py

+            return True
+
+        res = False
+        if type(self) is type(other) and \


I probably would have used == here but I don't think it matters.

mtreinish · 2021-02-11T20:50:32Z

qiskit/circuit/register.py

+                all(
+                        # For new-style bits, check bitwise equality.
+                        sbit == obit
+                        for sbit, obit in zip(self, other)
+                        if None in
+                        (sbit._register, sbit._index,
+                         obit._register, obit._index)
+                ):


I know I asked this in an earlier review, but was there performance impact here. In #5272 we removed the bit wise equality check because it had significant overhead for DAGNode creation. I don't think it will come up because it was per bit iirc and only an artifact of Bit.__eq__ previously comparing registers, but it's just something we should keep in mind.

Agree, much of the transpiler has been updated to avoid register comparisons in #5515 so it's unlikely to be an issue, but agree its something to keep an eye on.

mtreinish · 2021-02-11T20:50:54Z

releasenotes/notes/bit-classes-register-index-optional-9aa6feff4fce3524.yaml

+    The constructors of the :class:`~qiskit.circuit.Bit` class and subclasses,
+    :class:`~qiskit.circuit.Qubit`, :class:`~qiskit.circuit.Clbit`, and
+    :class:`~qiskit.circuit.AncillaQubit`, have been updated such that their
+    two parameters, ``register`` and ``index`` are now optional.


Maybe have an example here? But, we can just do it in the release time roundup.

Good point, I'll make sure to add it prior to release.

kdk added this to the 0.17 milestone Dec 8, 2020

kdk requested a review from a team as a code owner December 8, 2020 15:43

kdk added this to To do in Circuits via automation Dec 8, 2020

kdk added the optional-registers label Dec 8, 2020

mtreinish reviewed Jan 5, 2021

View reviewed changes

kdk force-pushed the quantumcircuit-optional-registers-api branch from 35ce723 to 734e48d Compare January 15, 2021 21:48

kdk added 5 commits January 20, 2021 16:48

Allow creation of Bits without requiring an associated Register.

0f56f6a

Add tests covering change in Bit.__eq__ behavior.

e0b9899

Register.__init__ to optionally accept list of Bits.

4ae52a9

QuantumCircuit.__init__ to optionally accept a list of Bits.

3b5e9fe

Add DAGCircuit.add_{qubit,clbit}s methods.

d18b0e2

kdk force-pushed the quantumcircuit-optional-registers-api branch from 734e48d to d18b0e2 Compare January 20, 2021 21:48

kdk force-pushed the quantumcircuit-optional-registers-api branch from 3b53fee to d18b0e2 Compare February 4, 2021 23:56

mtreinish reviewed Feb 11, 2021

View reviewed changes

Update qiskit/circuit/bit.py

4fca890

mtreinish approved these changes Feb 11, 2021

View reviewed changes

Merge branch 'master' into quantumcircuit-optional-registers-api

45843d8

kdk added the automerge label Feb 12, 2021

mergify bot merged commit 9040bb9 into Qiskit:master Feb 12, 2021

Circuits automation moved this from To do to Done Feb 12, 2021

mtreinish mentioned this pull request Feb 17, 2021

Performance regression with optional registers #5868

Open

mtreinish mentioned this pull request Mar 9, 2021

Swap mapper regression from stable to master #5997

Closed

kdk mentioned this pull request Mar 18, 2021

Fix AncillaQubits added via .add_bits to be included in .qubits. #6051

Merged

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

jakelishman mentioned this pull request Sep 15, 2023

Fix deprecated behaviour in timeline drawer #10846

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API changes for optional registers in QuantumCircuit. #5486

API changes for optional registers in QuantumCircuit. #5486

kdk commented Dec 8, 2020

mtreinish left a comment

mtreinish Jan 5, 2021

kdk Jan 6, 2021

mtreinish Jan 6, 2021

kdk Jan 15, 2021

mtreinish Jan 5, 2021

kdk Jan 6, 2021

mtreinish Jan 6, 2021

kdk Jan 15, 2021

mtreinish Jan 5, 2021

kdk Jan 6, 2021 •

edited

kdk Jan 15, 2021

mtreinish Jan 5, 2021

kdk Jan 6, 2021

mtreinish Jan 6, 2021

kdk commented Jan 6, 2021

mtreinish left a comment

mtreinish Feb 11, 2021

mtreinish Feb 11, 2021

kdk Feb 12, 2021

mtreinish Feb 11, 2021

kdk Feb 12, 2021

		@@ -43,6 +44,61 @@ def test_circuit_constructor_wires_wrong_mix(self):
		self.assertRaises(CircuitError, QuantumCircuit, 1, ClassicalRegister(2))


		class TestAddingBitsWithoutRegisters(QiskitTestCase):

		if (self._register, self._index) == (None, None):
		return "%s(%s)" % (self.__class__.__name__, hex(id(self)))

API changes for optional registers in QuantumCircuit. #5486

API changes for optional registers in QuantumCircuit. #5486

Conversation

kdk commented Dec 8, 2020

Summary

Details and comments

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

kdk Jan 6, 2021 • edited

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

kdk commented Jan 6, 2021

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

kdk Jan 6, 2021 •

edited