Polish noise docs #3569
Polish noise docs #3569
Conversation
|
Thanks @lamberta! Just addressed these. |
There was a problem hiding this comment.
Looks good on the whole, but it would be great if we could get more examples of how to build a noisy channel, currently I cant see this. But, very much like that we mention how to extract Kraus operators up-front - I didn't know this was possible.
Cheers,
Tom.
| "* A subset of channels which support `cirq.channel` also support the `cirq.mixture` protocol.\n", | ||
| " - If magic method `_mixture_` is not defined, `cirq.mixture` looks for `_unitary_`.\n", | ||
| "* A subset of channels which support `cirq.mixture` also support the `cirq.unitary` protocol.\n", | ||
| " - If `_unitary_` is not defined for an object, the object is probably not meant for a circuit!" |
There was a problem hiding this comment.
Not sure I understand this - do we expect all channels to have a unitary function? Or do you mean here that 'if a channel does not have a unitary function it is intended only to decorate a circuit e.g. as a noise model for a DensityMatrixSimulator'.
There was a problem hiding this comment.
Doubly confused now, because we use an amplitude_damping channel below in a circuit. So now I'm not really sure what this sentence means.
There was a problem hiding this comment.
The inclusions should be reversed - something in a circuit must have a channel, not necessarily a mixture or a unitary, though. Some channels have mixtures (e.g. cirq.bit_flip), and some channels have unitaries (e.g. cirq.X).
So, {channels with unitaries} ⊂ {channels with mixtures} ⊂ {channels}.
Thanks for pointing this out, I'm sure others who read it will have the same confusion. I'll add an example to hopefully clarify.
There was a problem hiding this comment.
Alright, perhaps remove the last line here though? "If _unitary_ is not defined for an object, the object is probably not meant for a circuit!" sounds like I can't add objects that aren't unitary to a circuit, which is confusing.
| "id": "99ac77a2c896" | ||
| }, | ||
| "source": [ | ||
| "Other than these general methods, channels can be added to circuits at any moment just as gates are. The channels can be different, be correlated, act on a subset of qubits, be custom defined, etc." |
There was a problem hiding this comment.
If I remember rightly, it should be possible to generate a 'noisy_moment' function that takes in a moment and adds noise to the entire chip. This could be in principle useful (e.g. if one wants to simulate crosstalk by adding unwanted rotations to nearby gates). Can we include mention + perhaps an example of this?
There was a problem hiding this comment.
Do you mean the Circuit.with_noise method (example in previous cell) or something else?
There was a problem hiding this comment.
I mean the NoiseModel.noisy_moment and NoiseModel.noisy_moments methods. These can be quite useful in some cases (e.g. the example I gave).
| "$$\\rho \\rightarrow \\sum_k p_k U_k \\rho U_k^\\dagger {\\rm ~where~} \\sum_k p_k =1 {\\rm ~and~ U_k U_k^\\dagger= I}$$\n", | ||
| "\n", | ||
| "In this case, it is possible to perform **Monte Carlo simulations** of these gates using a wave function based simulator (and not a density matrix based simulator). An example of this can be seen below: \n" | ||
| "To see the Kraus operators of a channel, the `cirq.channel` protocol can be used." |
There was a problem hiding this comment.
we could also link to the Protocols page from somewhere - maybe here or in a new sentence for users who don't understand what a protocol is.
There was a problem hiding this comment.
Sounds good. Is there a standard protocol (ha ha ha) to link to docs from docs? Should it be a devsite path like /cirq/protocols or link to GitHub/elsewhere?
There was a problem hiding this comment.
If you use the full API symbol in backticks it will autolink to the reference page on the site.
For narrative docs, if in the same project/repo you can relative link to the file (w/ extension), like [foo guide](./foo_guide.ipynb).
But if linking to a doc in another project/repo, need the absolute site path. This keeps the links working in GitHub and Colab.
|
Given the time pressure on this, I'm going to go ahead and merge - @obriente or @mpharrigan - if you have follow-up ideas those will be super welcome as an issue and we'll fix it up soon. Most comments are addressed though as far as I can tell. |
balopat
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
|
I left some comments in the review, is that sufficient or should I start
another issue?
Cheers,
Tom.
…On Thu, Dec 3, 2020 at 11:14 PM Cirq Bot ***@***.***> wrote:
Merged #3569 <#3569> into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3569 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD7ADDRKJMDGS4I7UMOZXV3STAES7ANCNFSM4ULK3VMA>
.
|
|
Thanks @obriente! No need for another issue, I will open a follow-up PR to address all remaining comments here and ping you to take another look. |
This PR addresses remaining comments from #3569 which was merged due to time constraints. - Adds link to protocols guide (and custom gates guide) to address comment from @balopat. - Adds example using `cirq.NoiseModel` and addresses other remaining comments from @obriente. Also adds section headers to density matrix simulation and monte carlo simulation to make them more apparent in the nav.
Revamped as per discussions with @karlunho.
Let me know what you think! cc @balopat @lamberta as this may be time-sensitive