Skip to content

feat: Classical control flow with mid circuit measurements#347

Merged
yitchen-tim merged 51 commits intomainfrom
mcm-experimental
Apr 13, 2026
Merged

feat: Classical control flow with mid circuit measurements#347
yitchen-tim merged 51 commits intomainfrom
mcm-experimental

Conversation

@speller26
Copy link
Copy Markdown
Member

Issue #, if available:

Description of changes:

Testing done:

Merge Checklist

Put an x in 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

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@yitchen-tim yitchen-tim self-requested a review February 19, 2026 22:02
* Squeeze single-element density matrix before computing expectation
* Pinned setuptools to fix doc build
* Fixed all linter errors
@speller26 speller26 changed the base branch from main to fix February 21, 2026 00:33
Base automatically changed from fix to main February 23, 2026 17:29
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (e6bc3bc) to head (f289b82).
⚠️ Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/braket/default_simulator/openqasm/program_context.py Outdated
Comment thread src/braket/default_simulator/gate_operations.py Outdated
Comment thread src/braket/default_simulator/openqasm/program_context.py Outdated
Comment thread src/braket/default_simulator/simulation_strategies/batch_operation_strategy.py Outdated
Comment thread src/braket/default_simulator/simulation_strategies/single_operation_strategy.py Outdated
Comment thread src/braket/default_simulator/simulator.py Outdated
@yitchen-tim
Copy link
Copy Markdown
Contributor

yitchen-tim commented Mar 2, 2026

Some product-level UX questions:

  • Several new feature for MCM triggers _uses_advanced_language_features=True. This causes warning "This program uses OpenQASM language features that may not be supported on QPUs or on-demand simulators." This is somewhat confusing to users, as if there is something wrong in their program/execution and they cannot trust the simulation results. Given that we are pushing for official support of MCM. Should we remove these warnings for MCM, and reserve _uses_advanced_language_features=True for something truly experimental?
  • We are lacking result schema and result class that supports MCM and general variable results. Right now, variables play no role in the result object. Result object always returns the end-of-circuit measurement outcome for all used qubits regardless whether there is an measurement instruction. Is it intentional and expected? Have you thought about the UX around the fact that we are not outputting any variables?

Copy link
Copy Markdown
Contributor

@laurencap laurencap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1.5 files today. Should have more time tomorrow!

Comment thread src/braket/default_simulator/openqasm/interpreter.py Outdated
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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/braket/default_simulator/openqasm/interpreter.py
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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, break and continue actually weren't supported at all before, even for static circuits

Comment thread src/braket/default_simulator/openqasm/program_context.py Outdated
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:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_branched and circuit properties won't be called until interpretation is complete?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_is_branched ia called frequently, but _pending_measurements will always be empty once the simulation is in branched mode (although that can be optimized).

Comment thread src/braket/default_simulator/gate_operations.py Outdated
Comment thread src/braket/default_simulator/gate_operations.py Outdated
Comment thread src/braket/default_simulator/gate_operations.py Outdated
Comment thread src/braket/default_simulator/gate_operations.py Outdated
Comment thread src/braket/default_simulator/openqasm/program_context.py
Comment thread src/braket/default_simulator/openqasm/program_context.py
MCM tests now all start from an OpenQASM program.
Comment thread src/braket/default_simulator/openqasm/interpreter.py
@yitchen-tim yitchen-tim closed this Mar 9, 2026
@yitchen-tim yitchen-tim deleted the mcm-experimental branch March 9, 2026 14:50
@yitchen-tim yitchen-tim restored the mcm-experimental branch March 9, 2026 14:51
@yitchen-tim yitchen-tim reopened this Mar 9, 2026
@yitchen-tim
Copy link
Copy Markdown
Contributor

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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point in the visitor, there is no way to know whether the measurement is being used as a MCM right?

Copy link
Copy Markdown
Contributor

@yitchen-tim yitchen-tim Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear why the _active_path_indices variable is being overwritten here

Copy link
Copy Markdown
Contributor

@yitchen-tim yitchen-tim Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would a variable be shared across paths need to be updated? Wouldn't it be local to the path?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why make this branched. The variable may never be used for branching right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't why understand _active_path_indices is being overwritten here, shouldn't it be extended?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this logic repeated from the interpreter?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can try extracting this

)

# Apply projection matrix
projected_state = state.copy()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider njitting this as a follow up

@yitchen-tim yitchen-tim merged commit d93266f into main Apr 13, 2026
20 checks passed
@yitchen-tim yitchen-tim deleted the mcm-experimental branch April 13, 2026 20:57
@yitchen-tim yitchen-tim restored the mcm-experimental branch April 13, 2026 20:57
@yitchen-tim
Copy link
Copy Markdown
Contributor

Keeping the branch alive to continue to iterate on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants