Ensure QuantumCircuit.append validates captures in control-flow
#10974
Conversation
jakelishman
added
Changelog: New Feature
|
One or more of the the following people are requested to review this:
|
jakelishman
force-pushed
the
var/cf-append
branch
from
dd21696
to
8c9ae3d
Compare
Pull Request Test Coverage Report for Build 7039489096Warning: 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 does not yet add the implementation of `QuantumCircuit.store`, which will come later as part of expanding the full API of `QuantumCircuit` to be able to support these runtime variables. The `is_lvalue` helper is added generally to the `classical.expr` module because it's generally useful, while `types.cast_kind` is moved from being a private method in `expr` to a public-API function so `Store` can use it. These now come with associated unit tests.
This adds all the new `QuantumCircuit` methods discussed in the variable-declaration RFC[^1], and threads the support for them through the methods that are called in turn, such as `QuantumCircuit.append`. It does yet not add support to methods such as `copy` or `compose`, which will be done in a follow-up. The APIs discussed in the RFC necessitated making `Var` nodes hashable. This is done in this commit, as it is logically connected. These nodes now have enforced immutability, which is technically a minor breaking change, but in practice required for the properties of such expressions to be tracked correctly through circuits. A helper attribute `Var.standalone` is added to unify the handling of whether a variable is an old-style existing-memory wrapper, or a new "proper" variable with its own memory. [^1]: Qiskit/RFCs#50
This commit adds support to the `QuantumCircuit` methods `copy` and `copy_empty_like` for manual variables. This involves the non-trivial extension to the original RFC[^1] that variables can now be uninitialised; this is somewhat required for the logic of how the `Store` instruction works and the existence of `QuantumCircuit.copy_empty_like`; a variable could be initialised with the result of a `measure` that no longer exists, therefore it must be possible for variables to be uninitialised. This was not originally intended to be possible in the design document, but is somewhat required for logical consistency. A method `add_uninitialized_var` is added, so that the behaviour of `copy_empty_like` is not an awkward special case only possible through that method, but instead a complete part of the data model that must be reasoned about. The method however is deliberately a bit less ergononmic to type and to use, because really users _should_ use `add_var` in almost all circumstances. [^1]: Qiskit/RFCs#50
This adds an inner check to the control-flow operations that their blocks do not contain input variables, and to `QuantumCircuit.append` that any captures within blocks are validate (in the sense of the variables existing in the outer circuit). In order to avoid an `import` on every call to `QuantumCircuit.append` (especially since we're already eating the cost of an extra `isinstance` check), this reorganises the import structure of `qiskit.circuit.controlflow` to sit strictly _before_ `qiskit.circuit.quantumcircuit` in the import tree. Since those are key parts of the circuit data structure, that does make sense, although by their nature the structures are of course recursive at runtime.
jakelishman
force-pushed
the
var/cf-append
branch
from
8c9ae3d
to
de0201f
Compare
There was a problem hiding this comment.
Overall this LGTM, the integration of the various pieces fits nicely together in the circuit structure and makes sense to me. Just a few inline comments and questions. I still need to go through the tests and I want to make another pass through the docs. One thing I think we really want is a guide somewhere on how to work with classical stores and expressions more broadly as there are pieces in various docstrings you can piece together but documenting all the capabilities in a single place would be useful for users so we can point them to it to explain the capabilities.
|
|
||
| __slots__ = ("var", "name") | ||
|
|
||
| var: qiskit.circuit.Clbit | qiskit.circuit.ClassicalRegister | uuid.UUID |
There was a problem hiding this comment.
My type hinting syntax knowledge isn't very deep but does this define a instance attr or a class attr type? If I saw this as an attr definition it would be a class attr.
There was a problem hiding this comment.
Python treats this as a pure type hint attached to the class, and iirc, this is the way to make Sphinx easily pick up both the docstring and the type hint (but I don't remember exactly why I moved this).
A classvar isn't created because there's no assignment, and the type hint for a classvar that isn't created by the same line would be var: ClassVar[str] (or whatever).
| >>> from qiskit.circuit.classical import expr | ||
| >>> expr.is_lvalue(expr.lift(2)) | ||
| False |
There was a problem hiding this comment.
Are you thinking we'll doctest these code blocks? At least that's normally the only context I see the interpreter >>> prompt used typically.
There was a problem hiding this comment.
I wasn't thinking that necessarily, it's just how I tended to do this - if I do it as separate "input" and "output" blocks, the spacing looks really awkward, and it's harder to show multiple outputs in line (like in the next couple of examples). I think the rest of the circuit_classical documentation uses this style.
| @@ -89,6 +89,9 @@ class Bool(Type, metaclass=_Singleton): | |||
| def __repr__(self): | |||
| return "Bool()" | |||
|
|
|||
| def __hash__(self): | |||
| return hash(self.__class__) | |||
There was a problem hiding this comment.
Not that it matters for this, but is there an advantage to using self.__class__ here vs type(self)
There was a problem hiding this comment.
Honestly I don't really know why I went for one or the other. I can use type if you prefer - I don't think there's much difference for Python-space types (but could be forgetting something).
There was a problem hiding this comment.
Yeah, this doesn't matter and it's fine to keep it like this. I was just wondering if there was an advantage one way or the other because my instinct would be to use type(self).
| .. warning:: | ||
|
|
||
| If the circuit contains any local variable declarations (those added by the | ||
| ``declarations`` argument to the circuit constructor, or using :meth:`add_var`), they | ||
| will be **uninitialized** in the output circuit. You will need to manually add store | ||
| instructions for them (see :class:`.Store` and :meth:`.QuantumCircuit.store`) to | ||
| initialize them. |
There was a problem hiding this comment.
I feel like we could go either way on this. I assume you went for copying the uninitialized vars because we copy clbits and classical registers already and this is just consistent with that. This just feels a bit different because using store manually doesn't seem as common as measure.
There was a problem hiding this comment.
In the current data structures, a variable is initialised only by a store instruction in the data; it's not stored directly in the circuit metadata. In this form, if we want to copy the initialised operations, we'd have to locate the stores that initialise them and copy them over as well, which would mean that len(qc.copy_empty_like()) would not be reliably zero, which seems pretty weird to me.
| subclassing. It is likely to become a special-case instruction in later versions of Qiskit | ||
| circuit and compiler internal representations.""" |
There was a problem hiding this comment.
Is your thinking here we'll treat it as a special case like barrier?
My question is based around thinking about how we want to model classical stores in the target and how we represent the capability of a backend to work with Vars like this.
There was a problem hiding this comment.
What I was meaning by "special case" was that it has inherent data that influences the structure of the constructed DAG (as opposed to it being part of the standard qubits/clbits).
For modelling stores in the target, I honestly don't really know - I'm not really sure how much it's going to make sense attempting to model classical handling in the Target in our current setup, since there's likely all sorts of constraints that different hardware will have that are going to be largely unrepresentable to us.
| def get_var(self, name: str, default: type(...) = ...) -> expr.Var: | ||
| ... | ||
|
|
||
| # We use a _literal_ `Ellipsis` as the marker value to leave `None` available as a default. |
There was a problem hiding this comment.
Fwiw I did this rather than the more standard pattern of an object() as the default value so that the Sphinx docs wouldn't look gross.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
|
I've updated this fully as of the most recent review. For "user guide" documentation - I think the right place for that will be in the separate documentation repo now? The |
There was a problem hiding this comment.
This LGTM now, thanks for the updates. As for the docs it depends, the model I've been operating under is if you were going to write a standalone rst file that goes in the new documentation repo, but otherwise you can do it somewhere in the qiskit tree. But either way we can cover this in a separate PR as it's easy to add new docs in a follow up either in Qiskit or in the documentation repo.
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 an inner check to the control-flow operations that their
blocks do not contain input variables, and to
QuantumCircuit.appendthat any captures within blocks are validate (in the sense of the
variables existing in the outer circuit).
In order to avoid an
importon every call toQuantumCircuit.append(especially since we're already eating the cost of an extra
isinstancecheck), this reorganises the import structure ofqiskit.circuit.controlflowto sit strictly beforeqiskit.circuit.quantumcircuitin the import tree. Since those are keyparts of the circuit data structure, that does make sense, although by
their nature the structures are of course recursive at runtime.
Details and comments
Depends on #10962
Close #10925
For CI efficiency reasons ahead of Summit, this is now being merged as the rollup of several PRs. This PR now:
Storeinstruction #10946QuantumCircuit#10962QuantumCircuitcopy methods #10963See the additional summary fields of those PRs (which are just the commit messages of the components of this PR) for more detail on them.