Fix resource resolution in InstructionSet.c_if
#7255
Conversation
Pull Request Test Coverage Report for Build 1468227974
💛 - Coveralls |
|
On hold until #7123 merges, because while there won't be any merge conflicts, logically this PR will want to make a couple of changes to some error checking routines in that PR once it's in. The code here is still good for immediate review, though. |
This completely overhauls `InstructionSet.c_if` to use the containing
`QuantumCircuit` instance (if there is one) to resolve requests for
classical resources. This has several benefits:
1. The meaning of a integer is guaranteed to be the same between
instances of `QuantumCircuit` and `InstructionSet`.
2. There's no need for code duplication between the two.
3. With the resolution tied to pre-existing instance methods or
functions now, there's no need for each `InstructionSet` instance
to build up its own resolution graph over registers, which is a
speed boost.
4. It makes it possible for resolvers in the future to reject certain
requests for resources, if they cannot be satisfied. This can
easily be used by scoping constructs, to forbid access to extra
resource once the scope has closed, since `InstructionSet.c_if` can
mutate the contents after the fact.
Since `InstructionSet` is technically part of the public API, albeit not
one we expect people to actually be using, this deprecates the
`circuit_cregs` keyword argument, and maintains a compatibility layer so
existing calls will continue to work until the deprecation period
expires. All internal usages of `InstructionSet` use the new form.
There remains an unfortunate linear complexity in the new resource
checker, when seeing if a `ClassicalRegister` target is valid. This is
because there is currently no set-like interface to the registers stored
in any given circuit, unlike the bits. `QuantumCircuit` exposes its
lists of registers publically, so adding this map safely is non-trivial,
as it is simple for a user or subclasses to cause any new map to fall
out of sync with the list. This commit does not attempt to fix this,
but simply suffers the linear penalty for now; we expect there to be
only a few registers, so it should not be too onerous.
The previous form of `InstructionSet.c_if` had a hack to support a
limitation of `disassemble`. Previously, any single-bit conditions
where the bit was part of a register of size one would get automatically
promoted to be conditional on the register, not the bit. With
`QasmQobj`, single-bit conditions are indistinguishable from conditions
on size-one registers, so `InstructionSet.c_if` promoted them all to
register comparisons to try and make `disassemble(assemble(qc))` a
complete round-trip in those cases. Since `QasmQobj` cannot cope with
loose classical bits or overlapping registers, anyway, this was at best
a stop-gap measure, as not all circuits can be round-tripped already.
This commit removes the auto-promotion behaviour from
`InstructionSet.c_if`, but adds the logic to `disassemble` instead,
noting in the documentation that not all circuits will produce exactly
equivalent output when round-tripped. This is to make the "type system"
of classical conditions strict, which significantly simplifies the
resource management.
a51b116 to
cbf2617
Compare
|
Updated to account for the extra changes in #7123, so this is no longer on hold. |
There was a problem hiding this comment.
Overall this LGTM, I especially like the documentation improvements. Just a few inline comments and questions but mostly just minor things and existing quirks with this functionality.
I also don't think the linear scan for registers will be that bad in practice. It's not like there are circuits out there with thousands of registers, I'd say the typical use case is one classical register for all the measurement bits. In the worst case it'll be a handful but like never more than 10, probably nothing where we should have scaling issues.
| .. note:: | ||
|
|
||
| ``disassemble(assemble(qc))`` is not guaranteed to produce an exactly equal circuit to the | ||
| input, due to limitations in the :obj:`.QasmQobj` format that need to be maintained for |
There was a problem hiding this comment.
Does sphinx resolve this correctly? If so neat, I didn't realize we could short cut it like this.
There was a problem hiding this comment.
:obj:`.x` only displays what's after the . (I think), and in module/class a.b.C it has the search path:
a.b.C.xa.b.xa.xx- any prefix
.x
Sphinx is meant to warn if there are duplicate names that it could resolve to, but I've never actually tried that myself - if I know there are potentially duplicate names, I tend to use slightly longer forms because if it's confusing for me/Sphinx, it'll probably be confusing for the reader too. For example, in the release notes I used :meth:`.InstructionSet.c_if` to avoid conflict with Instruction.c_if, and that displays like this:
edit: also yes, it does resolve :obj:`.QasmQobj` correctly in those docs.
| .. note:: | ||
|
|
||
| ``disassemble(assemble(qc))`` is not guaranteed to produce an exactly equal circuit to the | ||
| input, due to limitations in the :obj:`.QasmQobj` format that need to be maintained for | ||
| backend system compatibility. This is most likely to be the case when using newer features | ||
| of :obj:`.QuantumCircuit`. In most cases, the output should be equivalent, if not quite | ||
| equal. |
There was a problem hiding this comment.
This is a good note to include, I'm always telling people this that the QC -> Qobj -> QC path isn't guaranteed to be lossless (most recently here: https://quantumcomputing.stackexchange.com/questions/21795/is-there-a-quantum-circuit-builder-package-that-allows-you-to-import-export-a-ci/21800#21800 ). Making this explicit in the documentation is a good idea.
| " when creating this InstructionSet." | ||
| ) | ||
| if self._resolver is not None: | ||
| classical = self._resolver(classical) | ||
| for gate in self.instructions: | ||
| gate.c_if(classical, val) | ||
| return self |
There was a problem hiding this comment.
It is a bit weird if we don't allow chaining here that we return self like this.
There was a problem hiding this comment.
Yeah, seems slightly odd. Perhaps we were thinking of adding more methods to Instruction/InstructionSet so you may want to do multiple things at once.
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
This makes it clearer that the requester is actually returning a resource for the caller to use, which makes it semantically more natural that it may track those resources as well.
"Summary"
This completely overhauls
InstructionSet.c_ifto use the containingQuantumCircuitinstance (if there is one) to resolve requests forclassical resources. This has several benefits:
The meaning of a integer is guaranteed to be the same between
instances of
QuantumCircuitandInstructionSet.There's no need for code duplication between the two.
With the resolution tied to pre-existing instance methods or
functions now, there's no need for each
InstructionSetinstanceto build up its own resolution graph over registers, which is a
speed boost.
It makes it possible for resolvers in the future to reject certain
requests for resources, if they cannot be satisfied. This can
easily be used by scoping constructs, to forbid access to extra
resource once the scope has closed, since
InstructionSet.c_ifcanmutate the contents after the fact.
Since
InstructionSetis technically part of the public API, albeit notone we expect people to actually be using, this deprecates the
circuit_cregskeyword argument, and maintains a compatibility layer soexisting calls will continue to work until the deprecation period
expires. All internal usages of
InstructionSetuse the new form.There remains an unfortunate linear complexity in the new resource
checker, when seeing if a
ClassicalRegistertarget is valid. This isbecause there is currently no set-like interface to the registers stored
in any given circuit, unlike the bits.
QuantumCircuitexposes itslists of registers publically, so adding this map safely is non-trivial,
as it is simple for a user or subclasses to cause any new map to fall
out of sync with the list. This commit does not attempt to fix this,
but simply suffers the linear penalty for now; we expect there to be
only a few registers, so it should not be too onerous.
The previous form of
InstructionSet.c_ifhad a hack to support alimitation of
disassemble. Previously, any single-bit conditionswhere the bit was part of a register of size one would get automatically
promoted to be conditional on the register, not the bit. With
QasmQobj, single-bit conditions are indistinguishable from conditionson size-one registers, so
InstructionSet.c_ifpromoted them all toregister comparisons to try and make
disassemble(assemble(qc))acomplete round-trip in those cases. Since
QasmQobjcannot cope withloose classical bits or overlapping registers, anyway, this was at best
a stop-gap measure, as not all circuits can be round-tripped already.
This commit removes the auto-promotion behaviour from
InstructionSet.c_if, but adds the logic todisassembleinstead,noting in the documentation that not all circuits will produce exactly
equivalent output when round-tripped. This is to make the "type system"
of classical conditions strict, which significantly simplifies the
resource management.
Details and comments
This PR isn't as big as it looks - it's 90% tests and documentation.
The idea of a "resource resolver" callback is better for consistency, but I also designed it to do double duty. The control-flow circuit builder interface would struggle to work safely with
c_if, becausec_ifmutates instructions after they've passed throughQuantumCircuit.append, which is where the builders hook in to build scopes and manage resources, but havingc_ifcall back to a resource manager lets them work safely again (and also ensures that you can't mess with instructions once the scope has closed).Clbitindexing inc_ifinconsistent with circuit indexing #7246 by hooking into theQuantumCircuitinstance to resolve resources, not just the registers.c_ifoverwrite previouscondition#7247 by updating the documentation ofInstructionSet.c_ifandInstruction.c_if.c_ifcall linear in number of circuitClbits #7249 by indexing intoQuantumCircuit's bit lists, not constructing a new one each time.c_iffails when using indexing if there are no classical registers in the circuit #7250 by hooking into theQuantumCircuitinstance to resolve resources, not just the registers.disassemble/c_ifsize-one register promotion.