Skip to content

Adding noise statements to squin #211

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

Merged
merged 4 commits into from
May 5, 2025
Merged

Conversation

johnzl-777
Copy link
Contributor

@johnzl-777 johnzl-777 commented Apr 25, 2025

Some initial friction points/anticipated changes I can immediately see in translating this to stim

  • The original PauliChannel definition from @kaihsin can take a variable number of float arguments, but in the stim dialect there are explicit fields for each probability of a 1Q/2Q pauli operator being applied. My feeling is that this should be reflected on the squin side as just something like 1QPauliChannel and 2QPauliChannel with explicit fields. I don't think we'd lose any generality here along with the added benefit that it would be much harder for a user to misinput information (the default definition assumes you plug in either 3 or 15 values via varargs but it seems to easy to make a mistake here)
    • If we DO want varargs I'm not quite sure how to nicely feed this to the kirin @wraps decorator
  • From New Squin statements.  #200 , I see it could be possible to not have to provide an operator for basis and instead use a CliffordString (although I'd have to restrict CliffordString even further, to just the Pauli Operators. Would it be worth having something like a PauliString? Or is that silly?)

cc: @weinbe58 @Roger-luo @kaihsin

@johnzl-777 johnzl-777 linked an issue Apr 25, 2025 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Apr 25, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
6335 5451 86% 0% 🟢

New Files

File Coverage Status
src/bloqade/squin/noise/_init_.py 100% 🟢
src/bloqade/squin/noise/_dialect.py 100% 🟢
src/bloqade/squin/noise/stmts.py 100% 🟢
TOTAL 100% 🟢

Modified Files

File Coverage Status
src/bloqade/squin/_init_.py 100% 🟢
TOTAL 100% 🟢

updated for commit: f1cde89 by action🐍

Copy link

codecov bot commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@Roger-luo Roger-luo requested a review from Copilot April 25, 2025 23:25
Copilot

This comment was marked as spam.

@QuEraComputing QuEraComputing deleted a comment from Copilot AI Apr 29, 2025
@johnzl-777
Copy link
Contributor Author

This is the simplest port of the noise statements that @kaihsin originally produced.

I just have some observations/ideas that I'd appreciate getting feedback on (if it's worth it, I can make an RFC)

  • PauliChannel in this PR can take any number of floats but in Stim it boils down to either 3 values for a 1Q channel or 15 values for a 2Q channel. In the Stim dialect those numbers are fixed per statement. While I can certainly enforce the correct number of values during code generation I don't think it would hurt to just mirror that structure in squin.
  • Should PPError in squin map to the CORRELATED_ERROR...ELSE_CORRELATED_ERROR functionality in Stim? In which case, that should be added into the Stim dialect itself?

Copilot

This comment was marked as spam.

@johnzl-777 johnzl-777 marked this pull request as ready for review April 29, 2025 18:55
Copy link
Member

@Roger-luo Roger-luo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM can we have some basic tests on the constructors? Or maybe you could help @david-pl add the corresponding runtime in a new PR (which should test this too). I think would be nice to have it before next week's hackthon.

@david-pl
Copy link
Collaborator

david-pl commented May 2, 2025

@Roger-luo I'm happy to add the runtime. I'd just like to have #207 merged first.

Are there any plans for adding a rewrite pass that injects the noise as well?

@weinbe58
Copy link
Member

weinbe58 commented May 2, 2025

@Roger-luo + @david-pl I think we could simplify the runtime if we added a target which would be able to handle a wider variety of channels and would be easy to implement with PyQrack using the current runtime in #185

@statement(dialect=dialect)
class StochasticUnitaryChannel:
    operators: ir.SSAValue = argument(ilist.IListType[OpType, NumOperators]
    probabilities: ir.SSAValue = argument(ilist.IListType[float, NumOperators]

you could basically just rewrite all these other specialized noise channels to this statement and then the runtime would be just randomly select which operator to apply.

@johnzl-777 johnzl-777 merged commit b310f03 into main May 5, 2025
11 checks passed
@johnzl-777 johnzl-777 deleted the 166-add-noise-statements-to-squin branch May 5, 2025 17:28
@johnzl-777
Copy link
Contributor Author

From discussing with @Roger-luo and @weinbe58 I agree this PR is good to go (apologies again for sitting on this!) and just wanted to put down two notes:

  • @Roger-luo asked for some simple unit tests on the constructors, I'll make a separate PR for that
  • @weinbe58 's proposed additional statement will also go into a separate PR 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add noise statements to squin
5 participants