Refactor & simplify noise dialect #512
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
david-pl
added
breaking
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
weinbe58
reviewed
Oct 1, 2025
| assert all(0 <= x <= 1 for x in probs), "Invalid Pauli error probabilities" | ||
|
|
||
| for control, target in zip(controls, targets): | ||
| which = interp.rng_state.choice(self.two_pauli_choices, p=probs) |
Collaborator
Author
There was a problem hiding this comment.
Oh, yeah I kept that in mind here. This already fixes it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This simplifies the noise dialect a lot. I also cut down the supported semantics, so we now only support one- and two-qubit noise channels. Effectively, we now only have the following statements:
SingleQubitPauliChannelTwoQubitPauliChannelDepolarizeDepolarize2QubitLossTechnically,
Depolarize(2)andSingle(Two)QubitPauliChannelare redundant in that you can always rewriteDepolarizeto the other channel. But I didn't want to burden the squin to stim pass with matching that (you'd need to look at runtime values of probabilities to make sure things are symmetric) when rewriting toDepolarize. At the same time, we need the asymmetric versions for cirq lowering.One thing I realized: going the same stdlib approach as for the native and the clifford dialects, we end up needing
broadcastagain. And I didn't want to havesquin.channel.broadcast.depolarizeand similar things. So, for now, I just imported everything to the top level again, which means we havesquin.depolarizeandsquin.broadcast.depolarize.FYI @johnzl-777 I renamed the STIM tests, so they'll be skipped in the CI. If this gets merged before you get to the re-implementation of the squin to stim pass, keep that in mind.
Finally, I want to point out that it's not really possible to make the linter happy when going through a stdlib function that includes a list of probabilities, specifically
two_qubit_pauli_channel. The problem is, that the linter complains that[0.01, 0.02, ...]isn't anilist, but it's really lowered to one (the linter just doesn't know), so changing the type hint in the stdlib function tolist[float]errors since it's a kernel. That said, we need some special handling anyway to improve the API here (see also #341 ). But let's do this in another PR.