Added a dispatch based Noisify API ( For Issue #729)#750
Conversation
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Krastanov
left a comment
Krastanov
left a comment
There was a problem hiding this comment.
Looks overall good, need to do another pass of review to focus on a few things.
If you need to figure out type trees, use TypeTree.jl and do something like: println(join(tt(supertype(some_starting_type))))
| Idle noise requires the total number of qubits in the circuit to be specified. | ||
|
|
||
| ```@example noisify | ||
| noisy_circuit = noisify(circuit, noise_model; nqubits=2) |
There was a problem hiding this comment.
nqubits can be deduced by doing something like maximum(affectedqubits.(circuit)).
There was a problem hiding this comment.
This is helpful, but would this still work when the circuit does not touch all qubits in the provided stabilizer state? In that case, inferring nqubits from the circuit would undercount the register size, so idle noise would not be applied to qubits outside the maximum qubit index appearing in the circuit.
There was a problem hiding this comment.
I think that situation would be awkward to arise. Let's just warn in the docstring of idle noise / circuit noise that this is the case.
| ```@example noisify | ||
| register = Register(one(Stabilizer, 2), falses(1)) | ||
|
|
||
| mctrajectories(register, noisy_circuit; trajectories=100) |
There was a problem hiding this comment.
let's have two examples, one that uses pftrajectories as well
| reset = noise | ||
| ) | ||
|
|
||
| apply_idle_noise(circuit::AbstractVector, ::NoNoise, nqubits::Integer) = circuit |
There was a problem hiding this comment.
Can we rename it to insert_...?
| noisify(op::AbstractSingleQubitOperator, ::NoNoise) = Any[op] | ||
| noisify(op::AbstractTwoQubitOperator, ::NoNoise) = Any[op] | ||
| noisify(op::AbstractMeasurement, ::NoNoise) = Any[op] | ||
| noisify(op::Reset, ::NoNoise) = Any[op] |
There was a problem hiding this comment.
can't nonoise just have a single method?
There was a problem hiding this comment.
I had to introduce separate dispatch methods for NoNoise because i defined NoNoise as a subtype of AbstractNoise. Since noisify already has methods specialized on AbstractNoise, those methods would be selected over the generic NoNoise handler unless explicit NoNoise dispatches were provided for the affected operation types.
There was a problem hiding this comment.
ah, I guess this is something about method resolution ambiguities. Sounds good
|
|
||
| function noisify(op::AbstractSingleQubitOperator, noise_model::CircuitNoise) | ||
| out = Any[] | ||
| add_noise_op!(out, noise_model.single_qubit, affectedqubits(op)) |
There was a problem hiding this comment.
Why can't this just be noisify(op, noise_model.single_qubit)?
|
|
||
| function noisify(op::AbstractTwoQubitOperator, noise_model::CircuitNoise) | ||
| out = Any[] | ||
| add_noise_op!(out, noise_model.two_qubit, affectedqubits(op)) |
|
|
||
| function noisify(op::AbstractMeasurement, noise_model::CircuitNoise) | ||
| out = Any[] | ||
| add_noise_op!(out, noise_model.measurement, affectedqubits(op)) |
| function noisify(op::Reset, noise_model::CircuitNoise) | ||
| out = Any[] | ||
| push!(out, op) | ||
| add_noise_op!(out, noise_model.reset, affectedqubits(op)) |
There was a problem hiding this comment.
same, and also just curious about the change of order
…for noisify because it was being overridden by the abstract noise methods
| "Perturbative Expansions" => "noisycircuits_perturb.md", | ||
| "ECC example" => "ecc_example_sim.md", | ||
| "Circuit Operations" => "noisycircuits_ops.md", | ||
| "Noisify" => "noisify.md" |
There was a problem hiding this comment.
this probably needs a better name -- seeing this in the tables of content would not mean much to a reader
| layers = Dict{Int, Vector{Any}}() | ||
| active_qubits = Dict{Int, Set{Int}}() | ||
| for op in circuit | ||
| if op isa AbstractNoiseOp || op isa VerifyOp || op isa ClassicalXOR |
There was a problem hiding this comment.
Why is AbstractNoiseOp here?
More importantly, this seems to be not extendable. If a user or an external library creates a type that requires this same type of special treatment, they will not be able to use this noisify method.
Could you explain, maybe in a few drawings, why is this special cased in this fashion?
There was a problem hiding this comment.
Initially I special cased AbstractNoiseOp because I didn't want it to undergo idle noise (I'm still not sure if it is supposed to, do let me know). But I realize now that it can still follow the layering logic like a regular operation using affectedqubits, I'll make that change.
The part I'm still unsure about is how to handle ops such as ClassicalXOR or VerifyOp. The idle-noise insertion pass first schedules the circuit into timesteps/layers using the filled_up_to vector, so the resulting circuit is effectively reordered according to layer structure rather than preserving the original linear ordering. For ordinary operations this is fine. However, it’s not clear to me how VerifyOp and ClassicalXOR should interact with this scheduling pass. However, for VerifyOp and ClassicalXOR, I’m not sure what the correct scheduling semantics should be, or even how their timestep should be determined. Should they participate in the layer assignment process somehow, or should they instead preserve their position in the original circuit?
There was a problem hiding this comment.
The dilemma is as follows:
Suppose our circuit was: [H(1), ClassicalXOR(1), H(2)]
The current layering logic would have re order this according to the time steps as:
t=1: H(1), H(2)
t=2:ClassicalXOR(...)
In other words, it doesn't take place in any of the layering logic and is just placed in the current maximum timestep.
Another possible model could be that the circuit should be re-ordered as:
t=1: H(1)
t=2: ClassicalXOR(...)
t=3:H(2)
and similarly for VerifyOp.
The issue is that, unlike ordinary quantum operations, how should I assign a timestep to ClassicalXOR and VerifyOp and other possible ops in the future. As a result, I’m unsure whether they should participate in the layering process at all, or whether they should instead preserve their position in the original circuit.
Let me know if I was able to get my thoughts across clearly!
| layers = Dict{Int, Vector{Any}}() | ||
| active_qubits = Dict{Int, Set{Int}}() |
There was a problem hiding this comment.
What is the meaning of these two? Why are they needed in addition to filled_up_to?
There was a problem hiding this comment.
In the insert_idle_noise function, I use these layers and active qubits in line 64 in order to compute idle qubits for each time step layer ( this function makes the assumption that each gate only takes up one step in time, let me know if this is inaccurate). During the initial pass through the circuit, filled_up_to tracks the next available layer for each qubit, allowing operations to be assigned to their respective layers. layers records which operations occur in each layer so that the circuit can be reconstructed after idle-noise insertion. active_qubits records which qubits participate in operations within each layer. While active_qubits is not strictly necessary (it could be recomputed from layers), storing it avoids additional work during the reconstruction pass.
Krastanov
left a comment
Krastanov
left a comment
There was a problem hiding this comment.
looks good, some very minor things to change, and fixing the doc build
I want to see the docs before merging.
| Reset(reset_state, [2]), | ||
| VerifyOp(one(Stabilizer, 1), [2]), | ||
| ] | ||
| noisy = noisify(circuit, PauliNoise(1e-3, 1e-3, 1e-3)) |
There was a problem hiding this comment.
when the doc ci is fixed, I want to look at how this renders in the docs
|
|
||
| When the `idle_noise` option is configured in `CircuitNoise`, noise is inserted on qubits that are not participating in any operation during a given circuit layer. | ||
|
|
||
| Idle noise requires the total number of qubits in the circuit to be specified. |
| The same noisy circuit can also be simulated using pauli-frame trajectories. | ||
|
|
||
| ```@example noisify | ||
| register = Register(one(Stabilizer, 2), falses(2)) |
There was a problem hiding this comment.
pftrajectories does not take a register -- you can probably just directly give it the circuit
| noisify(op::AbstractNoiseOp, noise::AbstractNoise) = Any[op] | ||
| noisify(op::ClassicalXOR, noise::AbstractNoise) = Any[op] | ||
| noisify(op::VerifyOp, noise::AbstractNoise) = Any[op] |
There was a problem hiding this comment.
these do not seem to be used, unless there is some method ambiguity -- if there is method ambiguity, leave a comment mentioning ambiguity with which other method
There was a problem hiding this comment.
a simpler solution to fixing all the ambiguity is to NOT use NoNoise <: AbstractNoise, but rather to just use Nothing
|
|
||
|
|
||
| function noisify(circuit::AbstractVector, noise_model::CircuitNoise) | ||
| if noise_model.idle_noise isa NoNoise |
There was a problem hiding this comment.
generally never use isa in Julia -- it makes things unextendable. It is there for the rare need for introspection.
There was a problem hiding this comment.
thanks to line 29, you can just skip this
|
|
||
| insert_idle_noise(circuit::AbstractVector, ::NoNoise) = circuit | ||
|
|
||
| function insert_idle_noise(circuit::AbstractVector, idle_noise::AbstractNoise) |
There was a problem hiding this comment.
adding CNOT 2 3 to a circuit for which filled_up_to looks like this:
* * * * *
*
* * *
would result into a:
filled_up_to is modified to
* * * * *
* / / * (where / denotes "skip")
* * * *
idling noise will be added to each skip with push!(noisy_circuit, idlenoiseop)
push!(noisy_circuit, sCNOT(2,3))
And then for some specific operations, it just directly goes to push! without looking at filled_up_to. Instead of isa though, lets use a private method skip_idling_noise(_) = false and skip_idling_noise(::ParticularType) = true. Keep the method private but documented.
2 participants
Key Notes:
Most of the code was written by me but was passed through an LLM to restructure. In particular, the function apply_idle_noise was revised with LLM help. I've checked and verified the functionality of it, and I'll make sure that the logic is clearly documented
Fixes #729