-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add noise model #572
Add noise model #572
Conversation
Codecov Report
@@ Coverage Diff @@
## master #572 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 84 86 +2
Lines 12737 12861 +124
==========================================
+ Hits 12737 12861 +124
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this. In terms of features, this looks complete to me. As far as I understand #567 is mostly focused on the API of adding noise to circuits which is implemented here. One straightforward addition one could propose is to extend the list of noise types, however this can be done later in collaboration with theorists/experimentalists.
In terms of code, my main concern is the use of circ.queue.insert
, which may have some unwanted behavior. The issue is that queue
is not a Python list but a custom _Queue
object defined in abstractions/circuit.py and inserting elements to it using methods other than .append
may break some of its features. For example, in your example, if you do
print(circuit.depth)
print(noisy_circuit.depth)
you will see that both are 6, even though the noisy circuit is deeper.
Not related to this PR, but to avoid such issues in the future, perhaps we should remove the inheritance of _Queue
from list
or rename circuit.queue
to circuit._queue
to discourage modifying this outside the circuit.
Regarding this PR, one easy solution is to recreate the noisy circuit in NoiseModel.apply
, instead of using copy, eg.:
def apply(self, circuit):
circ = circuit.__class__(**circuit.init_kwargs)
for gate in circuit.queue:
circ.add(...) # add channels associated with the gate's noisy
circ.add(gate) # add the gate
Perhaps this would also simplify the code a bit as you do not need to keep track of num
and count
to insert the noise in the proper place.
Some other comments below:
The example in the first post misses some imports:
from qibo import gates
from qibo.models import Circuit
from qibo.noise import PauliError, ThermalRelaxationError, NoiseModel
Thanks for the review @stavros11. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates.
I checked the test issue and why it fails only for density matrix and I believe the behavior is expected. If you don't copy the initial state before passing it to the circuits, the three variables: initial_state
, final_state
and target_final_state
will all point to the same array in memory. So
K.assert_allclose(final_state, target_final_state)
performs a meaningless comparison that this array equals itself and always passes, even if target_circuit
is different than circuit
. This is not true when using density matrix because the PauliNoiseChannel
uses numpy addition which silently creates a copy, so final_state
and target_final_state
point to different arrays (as they should) and the test fails.
Generally it is a good idea to use a copy when planning to use the same initial state on different circuits on the same script, otherwise the in-place updates may lead to strange behavior as the above.
Regarding this PR, everything looks good to me now. If documentation is added it should be good to go.
I've added the documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the docs, looks good to me, some minor comments below.
After reading the updated docs, I believe that at this stage we could remove completely the circuit.with_noise
method, since that the new NoiseModel
replaces all its functionality and is more clear and flexible. @andrea-pasquale, @scarrazza what do you think?
Co-authored-by: Stavros Efthymiou <35475381+stavros11@users.noreply.github.com>
Thanks for the review. I've implemented all your suggestions.
Regarding |
I agree, we can keep the |
Closes #567.
As already discussed in #567 this PR implement a noise model interface in Qibo.
The noise model implement a map that associate a quantum channel Q to a specific gate G. In particular, the quantum channel Q is added before every instance of the gate G. It is also possible specify for which qubits the noise will be added.
Here is an example
EDIT: I've updated the current noise model API:
noise.add
supports directly the qibo gates no longer the qasm representation.with output
I mark this as WIP in case we want to implement other features related to the noise model in this PR.
This PR is also missing documentation.