API changes for optional registers in QuantumCircuit.

[kdk]

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:

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)

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.

[mtreinish]

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

[kdk]

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.

[mtreinish]

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

[kdk]

Comment added in 528f8593f8e2182f9d71303ba11ba8dac44a22b0 .

[mtreinish]

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

[kdk]

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.

[mtreinish]

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.

[kdk]

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]

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.

[kdk]

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.

[kdk]

Test added in 71ec0a4 .

[mtreinish]

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.

[kdk]

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.

[mtreinish]

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]

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).

[mtreinish]

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

[mtreinish]

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.

[kdk]

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]

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

[kdk]

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