Mid-circuit measurement feedforward support - #63
Conversation
|
The relevant files for the buffer method are branched_evolve_operators.jl, branched_simulator_operators.jl, and somewhat of gate_kernels/sv_simulator/dm_simulator but those ones have a less new stuff and mostly just have some helper functions. Most of the circuit evolution with the branching struct is in the first two files. |
…te of functionality is.
|
I realized I made a major oversight in the classical assignment operation. I assumed that the right hand side is equal for all paths, when in reality it is not. So, I am trying to update that and will commit as soon as I get it fixed. |
|
@shah-rushil for the CI failures, you can merge the latest from https://github.com/amazon-braket/BraketSimulator.jl/tree/mcm-experimental into your fork - it should update the CI so it's only testing against supported Julia versions. |
|
There are still issues regarding variable scoping and return statements that I am trying to fix. For return statements, it works if the return statement is in the outer body of the function call but if it is in an if statement, it won't exit out of the function. For scoping, there are some nuances regarding which variables to store and remove. I will implement this by keeping a frame counter that indicates which frame level the variables were created in. |
…operations along with tests
… increase coverage
speller26
left a comment
There was a problem hiding this comment.
More to come, just starting to review
| """ | ||
| FreeParameter | ||
| FreeParameter(name::Symbol) -> FreeParameter | ||
| FreeParameter |
There was a problem hiding this comment.
nit: We actually use spaces rather than tabs as convention; it keeps our line lengths consistent (the IDE can take care of auto-inserting the right number of spaces). This should also significantly reduce the number of lines in the PR
There was a problem hiding this comment.
Just a bump on this; this way, the reader can focus on the parts that changed in the file.
| println(expr) | ||
| println(sim.states) | ||
| println(sim.variables) | ||
| println(sim.qubit_mapping) | ||
| println(sim.measurements) |
There was a problem hiding this comment.
We can remove all the print statements
| # Process the expression to get qubit indices | ||
| expr_type = head(qubit_expr) | ||
|
|
||
| if expr_type == :identifier |
There was a problem hiding this comment.
Rather than going down a long chain of if branches, is it possible to dispatch using a dictionary? Same goes for inner if statements that check types.
| """ | ||
| FreeParameter | ||
| FreeParameter(name::Symbol) -> FreeParameter | ||
| FreeParameter |
There was a problem hiding this comment.
Just a bump on this; this way, the reader can focus on the parts that changed in the file.
| @@ -0,0 +1,51 @@ | |||
| # All of the functions that take in a Program type (JAQCD) since it is no longer used | |||
There was a problem hiding this comment.
Just making sure, is this just something you're moving out of BraketSimulator.jl or something like that? These paths are still used by the old non-branched simulator.
| end | ||
|
|
||
| """ | ||
| BranchedSimulatorOperators |
There was a problem hiding this comment.
If this struct is the simulator itself, then it should be named so (rather than ...Operators)
| qubit_name = Quasar.name(qubit_expr) | ||
|
|
||
| # Get the index | ||
| qubit_ix = _evolve_branched_ast_operators(sim, qubit_expr.args[2]) |
There was a problem hiding this comment.
Instead of working with general QasmExpressions, is there a better type we can work with? So instead of calling .args[2], which is hard to understand, the type will have a property that tells us what it is. This would also make the code far more robust against bugs that could be caused by indexing errors (or even worse, potential changes to QasmExpression structure).
| for i in 0:(length(state)÷2-1) | ||
| lower_ix = pad_bit(i, endian_target) + 1 | ||
| higher_ix = flip_bit(lower_ix - 1, endian_target) + 1 | ||
|
|
||
| # Swap amplitudes | ||
| temp = state[lower_ix] | ||
| state[lower_ix] = state[higher_ix] | ||
| state[higher_ix] = temp |
There was a problem hiding this comment.
Is it possible to reuse the X-gate logic here? Alternatively, change the existing X-gate logic to do this.
| Reset a qubit to |0⟩ state in a state vector simulator. | ||
| This projects the state to the |0⟩ state of the target qubit and then normalizes. | ||
| """ | ||
| function _apply_reset(simulator::StateVectorSimulator, target::Int) |
There was a problem hiding this comment.
Do we need both this and the preceding function?
| Apply measurement projection to a state vector simulator. | ||
| This collapses the state to the subspace corresponding to the measurement outcome. | ||
| """ | ||
| function apply_projection(simulator::StateVectorSimulator, qubit::Int, outcome::Int) |
| Reset a qubit to |0⟩ state in a density matrix simulator. | ||
| This projects the state to the |0⟩ state of the target qubit. | ||
| """ | ||
| function _apply_reset(simulator::DensityMatrixSimulator, target::Int) |
| return | ||
| end | ||
|
|
||
| function apply_gate!(::Reset, state_vec::AbstractStateVector{T}, qubit::Int) where {T<:Complex} |
There was a problem hiding this comment.
There's already an implementation for reset implemented in the simulator files; why have another here?
| As of now, this only works with registers and classical bits. Functionality for any classical variable | ||
| still needs to be implemented. | ||
| """ | ||
| function evaluate_qubits(sim::BranchedSimulator, qubit_expr::QasmExpression) |
There was a problem hiding this comment.
This is a really long function; it would be easier to reason about if it were split into multiple helpers
|
|
||
| Evaluates gate modifiers. Always returns a dictionary mapping path indices to tuples of (modifier_expr, inner_expr). | ||
| """ | ||
| function evaluate_modifiers(sim::BranchedSimulator, expr::QasmExpression) |
There was a problem hiding this comment.
This can be private too, since it's called by a private function
| end | ||
|
|
||
| """ | ||
| _apply_modifiers(gate_op::QuantumOperator, modifiers::Vector, target_indices::Vector{Int}) -> QuantumOperator |
There was a problem hiding this comment.
Would be good to distinguish the purpose of this from evaluate_modifiers; why do why have both, since the names suggest that they do the same thing?
|
|
||
| Handle a classical variable assignment in the branched simulation model. | ||
| """ | ||
| function _handle_classical_assignment(sim::BranchedSimulator, expr::QasmExpression) |
There was a problem hiding this comment.
Another very complex function that could be split up (much easier to reason about sub-functions than multiple nested conditions)
| current_paths = copy(sim.active_paths) | ||
|
|
||
| # This is for assigning to a bit register or any classical variable | ||
| if head(lhs) == :identifier |
There was a problem hiding this comment.
Another good place to use a dictionary rather than conditionals, since we're dispatching on types
speller26
left a comment
There was a problem hiding this comment.
Looks good! Just a few more minor comments.
| target_indices = target_indices_dict[path_idx] | ||
|
|
||
| # User-defined gate - create a new scope and execute the body | ||
| if haskey(sim.gate_defs, gate_name) |
There was a problem hiding this comment.
The two branches of this conditional definitely deserve dedicated functions; user-defined gates for sure deserve a handler considering their complexity, and a docstring explaining why it's so complex.
|
|
||
| # Convert the parameter results to a dictionary mapping path indices to parameter vectors | ||
| for path_idx in sim.active_paths | ||
| if param_results isa Dict && haskey(param_results, path_idx) |
There was a problem hiding this comment.
Rather than doing the isa Dict check each time, you can probably short-circuit that before entering the loop, since it'll evaluate to the same thing each time.
|
|
||
| for loop_value in loop_variable_vals | ||
| # Set the loop variable for each active path | ||
| for path_idx in sim.active_paths |
There was a problem hiding this comment.
A bit confusing to see path_idx as variable here shadowing the outer path_idx variable. In fact, does this even need to be a loop, since
sim.active_paths = [path_idx]
above?
| function _handle_casting(sim::BranchedSimulator, expr::QasmExpression) | ||
| casting_to = expr.args[1].args[1] | ||
| value = _evolve_branched_ast(sim, expr.args[2]) |
There was a problem hiding this comment.
Would probably be good to comment the type of node that's expected here, because it's hard to tell what value actually is
| function _handle_indexed_identifier(sim::BranchedSimulator, expr::QasmExpression) | ||
|
|
||
| identifier_name = Quasar.name(expr) | ||
| indices = _evolve_branched_ast(sim, expr.args[2]) |
There was a problem hiding this comment.
Would probably be good to comment the type of node that's expected here, because it's hard to tell what indices actually is
| if isa(gate_op, GPhase) | ||
| # Create a controlled version of the gate | ||
| # For phase gates, the control is the only qubit | ||
| bitvals = tuple(1) | ||
|
|
||
| # Create the controlled gate | ||
| gate_op = Control(gate_op, bitvals) | ||
| else | ||
| # Create a controlled version of the gate | ||
| # The first qubit is the control, the rest are targets for the gate | ||
| bitvals = tuple(1) | ||
|
|
||
| # Create the controlled gate | ||
| gate_op = Control(gate_op, bitvals) |
There was a problem hiding this comment.
Looks like this can just be collapsed, since both branches are the same (unless Julia typing doesn't allow it)
| if isa(gate_op, GPhase) | ||
|
|
||
| # Create a controlled version of the gate with control on |0⟩ state | ||
| bitvals = tuple(0) | ||
|
|
||
| # Create the negatively controlled gate | ||
| gate_op = Control(gate_op, bitvals) | ||
| else | ||
| # Create a controlled version of the gate with control on |0⟩ state | ||
| bitvals = tuple(0) | ||
|
|
||
| # Create the negatively controlled gate | ||
| gate_op = Control(gate_op, bitvals) | ||
| end |
| if is_measurement | ||
| # Process measurement outcomes for this path | ||
| if haskey(rhs_value.path_outcomes, current_path_idx) |
There was a problem hiding this comment.
For easier readability (fewer layers), combine these conditions, and make the else statement an elseif !is_measurement.
| if is_measurement | ||
| # Process measurement for this path | ||
| if haskey(rhs_value.path_outcomes, current_path_idx) |
There was a problem hiding this comment.
Same here with the conditional combining
speller26
left a comment
There was a problem hiding this comment.
Some readability suggestions; take or leave as you see fit
| if haskey(sim.qubit_mapping, indexed_name) | ||
| qubit_idx = sim.qubit_mapping[indexed_name] | ||
| # Store the qubit index directly | ||
| sim.variables[path_idx][param_name] = FramedVariable(param_name, :qubit_declaration, qubit_idx, false, sim.curr_frame+1) | ||
| else | ||
| error("Qubit $indexed_name not found in qubit mapping") | ||
| end |
There was a problem hiding this comment.
| if haskey(sim.qubit_mapping, indexed_name) | |
| qubit_idx = sim.qubit_mapping[indexed_name] | |
| # Store the qubit index directly | |
| sim.variables[path_idx][param_name] = FramedVariable(param_name, :qubit_declaration, qubit_idx, false, sim.curr_frame+1) | |
| else | |
| error("Qubit $indexed_name not found in qubit mapping") | |
| end | |
| haskey(sim.qubit_mapping, indexed_name) || error("Qubit $indexed_name not found in qubit mapping") | |
| qubit_idx = sim.qubit_mapping[indexed_name] | |
| # Store the qubit index directly | |
| sim.variables[path_idx][param_name] = FramedVariable(param_name, :qubit_declaration, qubit_idx, false, sim.curr_frame+1) |
| if haskey(sim.qubit_mapping, qubit_name) | ||
| qubit_idx = sim.qubit_mapping[qubit_name] | ||
| # Store the qubit index directly | ||
| sim.variables[path_idx][param_name] = FramedVariable(param_name, :qubit_declaration, qubit_idx, false, sim.curr_frame+1) | ||
| else | ||
| error("Qubit $qubit_name not found in qubit mapping") | ||
| end |
There was a problem hiding this comment.
| if haskey(sim.qubit_mapping, qubit_name) | |
| qubit_idx = sim.qubit_mapping[qubit_name] | |
| # Store the qubit index directly | |
| sim.variables[path_idx][param_name] = FramedVariable(param_name, :qubit_declaration, qubit_idx, false, sim.curr_frame+1) | |
| else | |
| error("Qubit $qubit_name not found in qubit mapping") | |
| end | |
| haskey(sim.qubit_mapping, qubit_name) || error("Qubit $qubit_name not found in qubit mapping") | |
| qubit_idx = sim.qubit_mapping[qubit_name] | |
| # Store the qubit index directly | |
| sim.variables[path_idx][param_name] = FramedVariable(param_name, :qubit_declaration, qubit_idx, false, sim.curr_frame+1) |
| if shots <= 0 | ||
| error("The number of inputted shots must be a positive integer") | ||
| end | ||
| if shots != simulator.shots[1] | ||
| error("The number of shots in the simulator must be equal to the number of shots passed in") | ||
| end |
There was a problem hiding this comment.
| if shots <= 0 | |
| error("The number of inputted shots must be a positive integer") | |
| end | |
| if shots != simulator.shots[1] | |
| error("The number of shots in the simulator must be equal to the number of shots passed in") | |
| end | |
| shots <= 0 || error("The number of inputted shots must be a positive integer") | |
| shots != simulator.shots[1] || error("The number of shots in the simulator must be equal to the number of shots passed in") |
| if !isempty(register_qubits) | ||
| for qubit_idx in register_qubits | ||
| push!(all_qubits, qubit_idx) | ||
| end | ||
| else | ||
| error("Missing qubit '$qubit_name'") | ||
| end |
There was a problem hiding this comment.
| if !isempty(register_qubits) | |
| for qubit_idx in register_qubits | |
| push!(all_qubits, qubit_idx) | |
| end | |
| else | |
| error("Missing qubit '$qubit_name'") | |
| end | |
| !isempty(register_qubits) || error("Missing qubit '$qubit_name'") | |
| for qubit_idx in register_qubits | |
| push!(all_qubits, qubit_idx) | |
| end |
| if ix >= 0 && ix < length(register_qubits) | ||
| push!(all_qubits, register_qubits[ix+1]) # Convert to 1-based indexing | ||
| else | ||
| error("Index $ix out of bounds for register $qubit_name") | ||
| end |
There was a problem hiding this comment.
| if ix >= 0 && ix < length(register_qubits) | |
| push!(all_qubits, register_qubits[ix+1]) # Convert to 1-based indexing | |
| else | |
| error("Index $ix out of bounds for register $qubit_name") | |
| end | |
| (ix >= 0 && ix < length(register_qubits)) || error("Index $ix out of bounds for register $qubit_name") | |
| push!(all_qubits, register_qubits[ix+1]) # Convert to 1-based indexing |
| if path_qubit_ix >= 0 && path_qubit_ix < length(register_qubits) | ||
| push!(all_qubits, register_qubits[path_qubit_ix+1]) # Convert to 1-based indexing | ||
| else | ||
| error("Index $path_qubit_ix out of bounds for register $qubit_name") | ||
| end |
There was a problem hiding this comment.
| if path_qubit_ix >= 0 && path_qubit_ix < length(register_qubits) | |
| push!(all_qubits, register_qubits[path_qubit_ix+1]) # Convert to 1-based indexing | |
| else | |
| error("Index $path_qubit_ix out of bounds for register $qubit_name") | |
| end | |
| path_qubit_ix >= 0 && path_qubit_ix < length(register_qubits) || error("Index $path_qubit_ix out of bounds for register $qubit_name") | |
| push!(all_qubits, register_qubits[path_qubit_ix+1]) # Convert to 1-based indexing |
| if julia_index <= length(var.val) | ||
| # Access the bit directly from the boolean array | ||
| results[path_idx] = var.val[julia_index] | ||
| else | ||
| error("Index out of bounds error, index too large for bit vector") | ||
| end |
There was a problem hiding this comment.
| if julia_index <= length(var.val) | |
| # Access the bit directly from the boolean array | |
| results[path_idx] = var.val[julia_index] | |
| else | |
| error("Index out of bounds error, index too large for bit vector") | |
| end | |
| julia_index <= length(var.val) || error("Index out of bounds error, index too large for bit vector") | |
| # Access the bit directly from the boolean array | |
| results[path_idx] = var.val[julia_index] |
| if haskey(sim.qubit_mapping, indexed_name) | ||
| qubit_idx = sim.qubit_mapping[indexed_name] | ||
| # Store the qubit index directly | ||
| sim.variables[path_idx][param_name] = ClassicalVariable(param_name, :qubit_declaration, qubit_idx, true) | ||
| else | ||
| error("Qubit $indexed_name not found in qubit mapping") | ||
| end |
There was a problem hiding this comment.
| if haskey(sim.qubit_mapping, indexed_name) | |
| qubit_idx = sim.qubit_mapping[indexed_name] | |
| # Store the qubit index directly | |
| sim.variables[path_idx][param_name] = ClassicalVariable(param_name, :qubit_declaration, qubit_idx, true) | |
| else | |
| error("Qubit $indexed_name not found in qubit mapping") | |
| end | |
| haskey(sim.qubit_mapping, indexed_name) || error("Qubit $indexed_name not found in qubit mapping") | |
| qubit_idx = sim.qubit_mapping[indexed_name] | |
| # Store the qubit index directly | |
| sim.variables[path_idx][param_name] = ClassicalVariable(param_name, :qubit_declaration, qubit_idx, true) |
| if haskey(sim.qubit_mapping, qubit_name) | ||
| qubit_idx = sim.qubit_mapping[qubit_name] | ||
| # Store the qubit index directly | ||
| sim.variables[path_idx][param_name] = ClassicalVariable(param_name, :qubit_declaration, qubit_idx, true) | ||
| else | ||
| error("Qubit $qubit_name not found in qubit mapping") | ||
| end |
There was a problem hiding this comment.
| if haskey(sim.qubit_mapping, qubit_name) | |
| qubit_idx = sim.qubit_mapping[qubit_name] | |
| # Store the qubit index directly | |
| sim.variables[path_idx][param_name] = ClassicalVariable(param_name, :qubit_declaration, qubit_idx, true) | |
| else | |
| error("Qubit $qubit_name not found in qubit mapping") | |
| end | |
| haskey(sim.qubit_mapping, qubit_name) || error("Qubit $qubit_name not found in qubit mapping") | |
| qubit_idx = sim.qubit_mapping[qubit_name] | |
| # Store the qubit index directly | |
| sim.variables[path_idx][param_name] = ClassicalVariable(param_name, :qubit_declaration, qubit_idx, true) |
| if expr_type in keys(evolution_dispatch_nodes) | ||
| return evolution_dispatch_nodes[expr_type](sim, expr) | ||
| else | ||
| error("Cannot process expression of type $expr_type.") | ||
| end |
There was a problem hiding this comment.
| if expr_type in keys(evolution_dispatch_nodes) | |
| return evolution_dispatch_nodes[expr_type](sim, expr) | |
| else | |
| error("Cannot process expression of type $expr_type.") | |
| end | |
| if expr_type in keys(evolution_dispatch_nodes) | |
| return evolution_dispatch_nodes[expr_type](sim, expr) | |
| error("Cannot process expression of type $expr_type.") |
3c80892
into
amazon-braket:mcm-experimental
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.