feat: Classical control flow with mid circuit measurements#347
feat: Classical control flow with mid circuit measurements#347yitchen-tim merged 51 commits intomainfrom
Conversation
More to come
* Squeeze single-element density matrix before computing expectation * Pinned setuptools to fix doc build * Fixed all linter errors
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #347 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 48 49 +1
Lines 4256 4754 +498
Branches 446 533 +87
==========================================
+ Hits 4256 4754 +498 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Some product-level UX questions:
|
laurencap
left a comment
There was a problem hiding this comment.
Reviewed 1.5 files today. Should have more time tomorrow!
| condition = cast_to(BooleanLiteral, self.visit(node.condition)) | ||
| for statement in node.if_block if condition.value else node.else_block: | ||
| self.visit(statement) | ||
| self.context.handle_branching_statement(node, self.visit) |
There was a problem hiding this comment.
Curious about this. Doesn't the visitor usually extract all the important fields, and then call context methods on extracted fields? Why not call visit from this visitor?
There was a problem hiding this comment.
This is really annoying, and I'm totally open to better ways to do this. The problem is, the node itself can contain MCM conditionals, so calling visit right away will trigger the branch hander; the recursive nature of these calls means evaluation is completed "bottom-up," while the branch simulations have to be done "top-down."
| measurement_target (Identifier | IndexedIdentifier | None): The AST node | ||
| for the classical variable being assigned, e.g. ``b`` in | ||
| ``b = measure q[0]``. Used by the branched MCM path to update | ||
| per-path classical variables. None for end-of-circuit measurements. |
There was a problem hiding this comment.
b[0] = measure q[0] is a valid circuit-final measurement right? In those cases, this is None. Is there no reason to track b[0] for circuit-final measurements?
I guess, because we return results by qubit index order at the end, it's not needed for anything?
There was a problem hiding this comment.
Renamed to classical_destination and clarified documentation; None guarantees that it's terminal because the result can't be used (it isn't assigned to anything); however, assignment to a target may be either terminal or feedforward, but we don't know until the result is actually used.
| def add_verbatim_marker(self, marker) -> None: | ||
| """Add verbatim markers""" | ||
|
|
||
| def handle_branching_statement(self, node: BranchingStatement, visit_block: Callable) -> None: |
There was a problem hiding this comment.
Cross commenting from the last file -- visit_block callback feels odd to me. Not that important though. Nice documentation here
| with self.enter_scope(): | ||
| self.declare_variable(node.identifier.name, node.type, i) | ||
| visit_block(deepcopy(node.block)) | ||
| except _BreakSignal: |
There was a problem hiding this comment.
I see, maybe these were moved for ease of integration with _Continue/BreakSignal? Is that basically what changed from interpreter.py, support for these signals?
There was a problem hiding this comment.
Yeah, break and continue actually weren't supported at all before, even for static circuits
| but no control flow depended on them) are registered in the circuit | ||
| as normal end-of-circuit measurements. | ||
| """ | ||
| if not self._is_branched and self._pending_mcm_targets: |
There was a problem hiding this comment.
is_branched and circuit properties won't be called until interpretation is complete?
There was a problem hiding this comment.
_is_branched ia called frequently, but _pending_measurements will always be empty once the simulation is in branched mode (although that can be optimized).
MCM tests now all start from an OpenQASM program.
|
Opened #351 to fix and allow re-measuring qubits. |
| ) | ||
| self.context.add_measure(qubits, targets) | ||
| if node.target and self.context.supports_midcircuit_measurement: | ||
| self.context.add_measure(qubits, targets, classical_destination=node.target) |
There was a problem hiding this comment.
at this point in the visitor, there is no way to know whether the measurement is being used as a MCM right?
There was a problem hiding this comment.
correct, supports_midcircuit_measurement is to activate the code path that the measurement is potentially a mcm, but does not commit to a branch simulation path.
But I do agree it's a bit confusing that the logic relys on the presence of classical_destination.
There was a problem hiding this comment.
Code path that relies on the presence of classical_destination seems fragile and hard to maintain. Is there a more explicit way?
| # so that expressions like ``y = x`` read from the correct path. | ||
| saved_active = list(self.context._active_path_indices) | ||
| for path_idx in saved_active: | ||
| self.context._active_path_indices = [path_idx] |
There was a problem hiding this comment.
It is unclear why the _active_path_indices variable is being overwritten here
There was a problem hiding this comment.
I think this is to handle the edge case that I added: test_get_value_reads_correct_path_in_shared_if_body . The program context always get value from the first active branch which would cause silent error. So here it tries to populate correct identifiers' value for active branches.
It does feels hacky this way.
There was a problem hiding this comment.
+1 to Aniket comment. We should avoid overwriting. Changing the logic that the program context always only get value from the first active branch may be the key to avoid that
| def update_value(self, variable: Identifier | IndexedIdentifier, value: Any) -> None: | ||
| """Update variable value, operating per-path when branched. | ||
|
|
||
| When branched, updates the variable on all active paths. Indexed |
There was a problem hiding this comment.
When would a variable be shared across paths need to be updated? Wouldn't it be local to the path?
There was a problem hiding this comment.
All active paths, namely the paths that are using this variable value.
| dest_name = mcm_dest.name if isinstance(mcm_dest, Identifier) else mcm_dest.name.name | ||
| if dest_name == name: | ||
| if not self._is_branched and self._shots > 0: | ||
| self._is_branched = True |
There was a problem hiding this comment.
why make this branched. The variable may never be used for branching right?
There was a problem hiding this comment.
After fixing the bug Tim pointed out, this might actually be a misnomer; flushing used to be done only on encountering control flow. This can be optimized, but I consider it a low priority because I don't think we're gonna see much use of MCMs outside of control flow.
|
|
||
| for path_idx in saved_active: | ||
| self._active_path_indices = [path_idx] | ||
| if cast_to(BooleanLiteral, self._visitor(node.condition)).value: |
There was a problem hiding this comment.
Here is the MCM used in the condition been flushed? Otherwise this casting will throw an exception right?
| false_paths = [] | ||
|
|
||
| for path_idx in saved_active: | ||
| self._active_path_indices = [path_idx] |
There was a problem hiding this comment.
I don't why understand _active_path_indices is being overwritten here, shouldn't it be extended?
There was a problem hiding this comment.
We're evaluating each path separately, so at this point we start evaluating everything that can happen starting from this point; any new branches only happen within this path. We can probably do some work to merge paths, but that gets real hairy and goes beyond the scope of the PR.
| if isinstance(index, RangeDefinition) | ||
| else index.values | ||
| ) | ||
| for i in index_values: |
There was a problem hiding this comment.
Isn't this logic repeated from the interpreter?
There was a problem hiding this comment.
Yeah, I can try extracting this
| ) | ||
|
|
||
| # Apply projection matrix | ||
| projected_state = state.copy() |
There was a problem hiding this comment.
Can't this be done inplace?
| if qubit_value != self.result: | ||
| projected_state[i] = 0 | ||
| # Zero out amplitudes that don't match the measurement result | ||
| for i in range(len(projected_state)): |
There was a problem hiding this comment.
consider njitting this as a follow up
|
Keeping the branch alive to continue to iterate on it |
Issue #, if available:
Description of changes:
Testing done:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.