Add QuantumCircuit.find_bit to find index and Registers for a Bit.

[kdk]

Summary

Resolves #6495 by replacing QuantumCircuit{._qubit_set,._clbit_set} with QuantumCircuit{._qubit_indices,._clbits_indices} which are dicts mapping Qubit and Clbit instances to a tuple of their index (in QuantumCircuit{.qubits,.clbits} and any containing Registers on the circuit.

Adds a method QuantumCircuit.find_bit to perform a lookup of a given Bit.

Details and comments

WIP:

• Replacing localized bit_indices calculations with QuantumCircuit.find_bit
• Resolve muted DeprecationWarnings from Deprecate Bit.index and Bit.register #6069 .

[mtreinish]

I think we also will need this for dags too, This comes up a lot in transpiler passes too. We can do that in a separate PR though

[kdk]

Due to the addition of QuantumCircuit{Q,C}regs this ended up larger than expected, so splitting out the DAGCircuit implementation and remaining bit_indices clean up in to subsequent PRs.

[jakelishman]

The find_bit function certainly seems useful given that Bit no longer stores references to its containing registers. I wasn't around at the time that decision was made - if you want to be able to locate registers from Bit instances in some form (like here), could you point me to any discussion of why to remove it? Was there a discussion about keeping weakrefs in the Bit? Perhaps they could have made this reverse look-up a bit simpler, without introducing nasty reference cycles for the garbage collector to sort out.

As it stands, I have a worry about separation of concerns between QuantumCircuit.add_register and QuantumCircuitQregs in a few list-modifying methods. QuantumCircuitQregs seems to suggest that modifying QuantumCircuit.qregs directly is now acceptable (since it calls back to QuantumCircuit), such as

In [9]: from qiskit import QuantumCircuit, QuantumRegister
   ...: qregs = [QuantumRegister(2) for _ in [None]*2]
   ...: qc = QuantumCircuit(qregs[0])
   ...: qc.qregs.append(qregs[1])
   ...: qc.x(range(4))
   ...: qc.draw()
Out[9]:
       ┌───┐
q12_0: ┤ X ├
       ├───┤
q12_1: ┤ X ├
       ├───┤
q13_0: ┤ X ├
       ├───┤
q13_1: ┤ X ├
       └───┘

However, I think in doing so the responsibilities of QuantumCircuitQregs and QuantumCircuit.add_register have become slightly crossed. Now, since Qregs is meant to appear like a list to the user, I'd expect to be able to do

In [2]: from qiskit import QuantumCircuit, QuantumRegister
   ...: qregs = [QuantumRegister(2) for _ in [None]*4]
   ...: qc = QuantumCircuit(qregs[0], qregs[1])
   ...: qc.qregs[2:] = qregs[2:]
---------------------------------------------------------------------------
CircuitError                              Traceback (most recent call last)
<ipython-input-1-98e2b42d9269> in <module>
      2 qregs = [QuantumRegister(2) for _ in [None]*4]
      3 qc = QuantumCircuit(qregs[0], qregs[1])
----> 4 qc.qregs[2:] = qregs[2:]

~/code/qiskit/terra/qiskit/circuit/quantumcircuitdata.py in __setitem__(self, key, value)
    155
    156     def __setitem__(self, key, value):
--> 157         self._circuit.add_register(value)
    158         self._list[key] = value
    159         del self._list[-1]

~/code/qiskit/terra/qiskit/circuit/quantumcircuit.py in add_register(self, *regs)
   1230
   1231             elif isinstance(register, list):
-> 1232                 self.add_bits(register)
   1233             else:
   1234                 raise CircuitError("expected a register")

~/code/qiskit/terra/qiskit/circuit/quantumcircuit.py in add_bits(self, bits)
   1253                 self._clbit_indices[bit] = BitLocations(len(self._clbits) - 1, [])
   1254             else:
-> 1255                 raise CircuitError(
   1256                     "Expected an instance of Qubit, Clbit, or "
   1257                     "AncillaQubit, but was passed {}".format(bit)

CircuitError: "Expected an instance of Qubit, Clbit, or AncillaQubit, but was passed QuantumRegister(2, 'q2')"

(Qregs.__setitem__ doesn't handle slices), or with negative indices:

In [6]: from qiskit import QuantumCircuit, QuantumRegister
   ...: qregs = [QuantumRegister(2) for _ in [None]*4]
   ...: qc = QuantumCircuit(qregs[0])
   ...: print(qc.qregs[-1] is qregs[0], qc.qregs[-1] is qregs[1])
   ...: qc.qregs[-1] = qregs[1]
   ...: print(qc.qregs[-1] is qregs[0], qc.qregs[-1] is qregs[1])
True False
True False

where the last line should be False True.

I mentioned it further down, but I think we need some way for Qregs to access only the (currently imaginary) QuantumCircuit._add_references_to_bits functionality separately to the QuantumCircuit._add_references_to_registers, since Qregs is now meant to be handling that.

[jakelishman]

Logically should this be map(str, [...]), even if __str__ calls __repr__?

[kdk]

Yup, this was copy paste from an existing test, but agree str makes more sense here.

[jakelishman]

Is there a reason to avoid repr([reg1, reg2, reg3, reg4])?

[kdk]

I don't think so, these should be equivalent. I'll update.

[jakelishman]

Is there a reason for leaving these abstract? With the docstring saying that this class is intended to implement list-like semantics, these would seem to have obvious default implementations, just like sort or copy.

[kdk]

That's true. I left the methods that mutated self._list as abstract because I couldn't see an obvious implementation that worked cleanly (across the few use cases we currently support) with validation, so this is left to the subclass.

[jakelishman]

Alternatively, would it be cleaner to factor out the validation into a separate, private instance method, and then just have __setitem__ and insert both call it?

[kdk]

Thinking about this a bit more, I think yes, but this would require a not inconsiderable refactor to both the circuit and register. We're lumping a few concerns behind the concept of "validation" (is the item being added a valid candidate member for this data structure, and following this addition/modification, is the data structure left in a consistent state) which are currently implemented throughout QuantumCircuit.add_register, QuantumCircuit.add_bits, and Register.__init__. If keeping these list-like interfaces is something we intend to support in the long term (and likely even if not), I agree there are definite opportunities for re-structuring to make this logic cleaner which I think we can tackle as a separate issue.

[jakelishman]

Could this be made the concrete definition in VerifiedMutableSequence? I can't necessarily see a case where del would need to do any subclass-specific value validation, and the other defined subclass below uses the same method.

[jakelishman]

Come to think of it, I can think of a case - If Qregs manages the registers, and a register is deleted from the list, it should presumably call back to the circuit to delete references to the contained Bit instances, and raise a noisy exception if any current instructions operate on those bits.

[kdk]

Good catch, I think there may be a case that I'm missing here (maybe removing a register that has old-style bits needs to be verboten until old-style bits are removed, or removing a register that's used in a conditional of a subsequent instruction) but if not, this could be made concrete.

[jakelishman]

The cyclical call structure here is difficult to follow for me. As I understand it:

1. self._circuit.add_register(value) ends up calling self.append(value)
2. now that the bit references are in place within self._circuit, we set the actual element of our list to the correct value
3. we delete the extra data that the unwanted self.append(value) added

My worries here are:

1. it feels like we actually just want something like self._circuit._add_internal_bit_references(value), since in the end, this class is responsible for the append of registers.
2. this won't work if key is negative, or a slice, both of which are supported by list syntax. For example, self[-1] = QuantumRegister(2) would append the quantum register to the end of the list, overwrite the same list location with the same data, and then delete it off the end. A slice would cause a list to get nested in the middle.

I think the solution to this involves changing the call to self._circuit.add_register, so that the method by which we register bits in the circuit does not affect self, and from there we can delegate to self._list.__setitem__ entirely. This would probably involve splitting out the "add to list of registers" logic from the "add references to all bits logic" inside QubitCircuit.add_register, so we can only call the latter.

[kdk]

The find_bit function certainly seems useful given that Bit no longer stores references to its containing registers. I wasn't around at the time that decision was made - if you want to be able to locate registers from Bit instances in some form (like here), could you point me to any discussion of why to remove it? Was there a discussion about keeping weakrefs in the Bit? Perhaps they could have made this reverse look-up a bit simpler, without have nasty reference cycles for the garbage collector to sort out.

Qiskit/RFCs#15 (though dated) outlines the motivation behind removing Register references from Bits, which focuses more on unifying the structure of QuantumCircuits and Instructions, so as to make the interface for building and composing each more consistent. Removing reference cycles was just a happy byproduct :) .

As it stands, I have a worry about separation of concerns between QuantumCircuit.add_register and QuantumCircuitQregs in a few list-modifying methods. QuantumCircuitQregs seems to suggest that modifying QuantumCircuit.qregs directly is now acceptable (since it calls back to QuantumCircuit),

The intention here was actually the opposite, that QuantumCircuitQregs exist only as a compatibility shim so that modifying QuantumCircuit.qregs continues to work (and to be consistent with QuantumCircuit.qubits). QuantumCircuitQregs should only ever defer to QuantumCircuit.add_register (though I agree there are opportunities for factoring this code more clearly). Migrating these interfaces (.qregs, .cregs, .data) to be read-only is another viable option, though as this was previously supported behavior, would still require a period of deprecation.

I mentioned it further down, but I think we need some way for Qregs to access only the (currently imaginary) QuantumCircuit._add_references_to_bits functionality separately to the QuantumCircuit._add_references_to_registers, since Qregs is now meant to be handling that.

This seems like the right direction, and I think a lot of your (valid!) concerns stem from the tangling of validation logic in places like QuantumCircuit.add_register so something like QuantumCircuit._add_references_to_{bits,registers} seem a valid path forward. But, as in #6621 (comment) , unless there are interface changes that would come from this, this might be better addressed as a separate issue.

[kdk]

I split out the ValidatedMutableSequence and QuantumCircuit{Q,C}regs changes into a separate PR ( #7008 ) and reported the issue they were trying to fix separately ( #7009 ), so this PR should be ready for review.

[jakelishman]

Most comments are typo fixes, and in principle everything looks good to me in this PR. My most major comments were whether we can de-duplicate some population of QuantumCircuit._qubit_indices by pushing some work onto QuantumCircuit.add_register (I think we can, without performance issues), and probably that we should use namedtuple attribute access rather than indexed access (unless it is critically slow on Python 3.7 - again, I think we should be ok).

[jakelishman]

Looks good to me - just needs a reno now.

I pushed up a minor change adding type information and Sphinx crossrefs to the documentation of find_bit, and removed the accidentally included test file (there's a bug in the visualisation tests that's generating an extra file at the moment), just in the interests of getting things merged quickly.

I'll approve properly after reno.

[jakelishman]

These more minimal changes look good to me now, so now there's nothing blocking #6565!