Track runtime variables in control-flow builders #10977
Conversation
jakelishman
added
the
mod: circuit
|
One or more of the the following people are requested to review this:
|
jakelishman
force-pushed
the
var/cf-builder
branch
from
6502ead to
bf30a73
Compare
Pull Request Test Coverage Report for Build 7051272536Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
This adds support for all the new classical runtime variables through the control-flow builder interface. In particular, most usefully it automatically manages the scoping rules for new declarations and inner variable accesses, and ensures that its built scopes automatically close over any variables used within them (including making sure nested scopes do the same thing). The builder API is factored out a little into an explicit interface object, with `QuantumCircuit` getting an explicit implementation of that. This is done because the number of separate API methods we would have needed to pass around / infer was getting overly large, and this allows us to just use standard virtual dispatch to automatically do the right thing. Python doesn't have a way to have an object implement an interface other than by structural (duck) typing, so to avoid name leakage and collisions, we instead make `QuantumCircuit`'s implementation a friend class that handles the inner state on its behalf. Not everything control-flow-builder related is factored out into the API because it didn't seem overly useful to do this, especially when the overridden behaviour would just have been to throw exceptions.
jakelishman
force-pushed
the
var/cf-builder
branch
from
bf30a73 to
0a81f75
Compare
There was a problem hiding this comment.
Overall this LGTM the changes are pretty sensible and straightforward once you wrap your head around the new interface for dealing with scoping. I had a couple of comments inline. The only thing that I found a bit hard to follow was referring to the current scope variable as api everywhere. I know what the intent there is, but I'd probably just call it scope, circuit_scope, or current_scope to make it a bit more explicit about what the variable is. But it's probably ok to stick with api everywhere I guess since I could follow it. The only really actionable comment is a tiny docs nit.
| forbidden_message: If a string is given here, a :exc:`.CircuitError` will be raised on | ||
| any attempts to append instructions to the scope with this message. This is used by | ||
| pseudo scopes where the state machine of the builder scopes has changed into a | ||
| position where no instructions should be accepted, such as when inside a ``switch`` | ||
| but outside any cases. | ||
| """ | ||
| self.instructions: List[CircuitInstruction] = [] | ||
| self._instructions: List[CircuitInstruction] = [] |
There was a problem hiding this comment.
This will probably conflict with: #11214 which I think is changing this to CircuitData.
There was a problem hiding this comment.
It will conflict, but in a way that's super easy to fix and honestly, #11214 unifies the types of the instructions circuit scope object in a much more pleasant way.
qiskit/circuit/quantumcircuit.py
Outdated
| broadcast_iter = ( | ||
| operation.broadcast_arguments(expanded_qargs, expanded_cargs) | ||
| if isinstance(operation, Instruction) | ||
| else Instruction.broadcast_arguments(operation, expanded_qargs, expanded_cargs) | ||
| ) | ||
| for qarg, carg in broadcast_iter: | ||
| self._check_dups(qarg) | ||
| instruction = CircuitInstruction(operation, qarg, carg) | ||
| api.append(instruction) | ||
| instructions._add_ref(api.instructions, len(api.instructions) - 1) |
There was a problem hiding this comment.
FWIW, this is kind of an unrelated change, it's more compact and easier to read. But really the only change was from appender -> api.append which took me a bit longer to see because of the other refactoring.
There was a problem hiding this comment.
The broadcast_iter is the only unrelated change. Originally this was only collapsing appender into api.append (since we already have the api object), but since #10827 the api.instructions field is dispatched as well, so it was growing to quite a few dispatched names.
Co-authored-by: Matthew Treinish <[email protected]>
This commit is a roll-up reversion of the following commits (PR): * This reverts commit a79e879 (Qiskit#10977) * This reverts commit ba161e9 (Qiskit#10974) * This reverts commit 50e8137 (Qiskit#10944) This is being done to clear the 1.0 branch of memory-owning `expr.Var` variables and `Store` instructions. It should be re-applied once 1.0 is released. The individual reverts had conflicts, since various other large-scale changes include wrap-ups of the changes to be reverted. This reversion should have handled all that, and other places where standalone `Var` nodes were referenced (such as in "See also" sections), even if not used.
This commit is a roll-up reversion of the following commits (PR): * commit a79e879 (Qiskit#10977) * commit ba161e9 (Qiskit#10974) * commit 50e8137 (Qiskit#10944) This is being done to clear the 1.0 branch of memory-owning `expr.Var` variables and `Store` instructions. It should be re-applied once 1.0 is released. This wasn't done by individual `revert` operations, because there were also significant structural changes introduced in those PRs that were very valid and should be maintained. Cross-references to `Var` nodes from other functions have been removed for now. Making `Var` and `types.Type` hashable is maintained, as is the `Var.standalone` function, in order to prepare the ground for the inclusion of proper `Var` nodes in a minor release.
This commit is a roll-up reversion of the following commits (PR): * commit a79e879 (#10977) * commit ba161e9 (#10974) * commit 50e8137 (#10944) This is being done to clear the 1.0 branch of memory-owning `expr.Var` variables and `Store` instructions. It should be re-applied once 1.0 is released. This wasn't done by individual `revert` operations, because there were also significant structural changes introduced in those PRs that were very valid and should be maintained. Cross-references to `Var` nodes from other functions have been removed for now. Making `Var` and `types.Type` hashable is maintained, as is the `Var.standalone` function, in order to prepare the ground for the inclusion of proper `Var` nodes in a minor release.
Summary
This adds support for all the new classical runtime variables through
the control-flow builder interface. In particular, most usefully it
automatically manages the scoping rules for new declarations and inner
variable accesses, and ensures that its built scopes automatically close
over any variables used within them (including making sure nested scopes
do the same thing).
The builder API is factored out a little into an explicit interface
object, with
QuantumCircuitgetting an explicit implementation ofthat. This is done because the number of separate API methods we would
have needed to pass around / infer was getting overly large, and this
allows us to just use standard virtual dispatch to automatically do the
right thing.
Python doesn't have a way to have an object implement an
interface other than by structural (duck) typing, so to avoid name
leakage and collisions, we instead make
QuantumCircuit'simplementation a friend class that handles the inner state on its
behalf.
Not everything control-flow-builder related is factored out into the API
because it didn't seem overly useful to do this, especially when the
overridden behaviour would just have been to throw exceptions.
Details and comments
Depends on #10963 and #10974
Close #10938