squin to stim rewrite

[johnzl-777]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 31.22924% with 207 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/bloqade/squin/rewrite/stim_rewrite_util.py	16.25%	67 Missing ⚠️
src/bloqade/squin/rewrite/squin_measure.py	21.81%	43 Missing ⚠️
src/bloqade/squin/rewrite/qubit_to_stim.py	22.22%	35 Missing ⚠️
src/bloqade/squin/rewrite/wire_to_stim.py	23.80%	32 Missing ⚠️
src/bloqade/squin/rewrite/wrap_analysis.py	57.14%	21 Missing ⚠️
...bloqade/squin/rewrite/wire_identity_elimination.py	41.66%	7 Missing ⚠️
src/bloqade/analysis/address/impls.py	66.66%	1 Missing ⚠️
src/bloqade/squin/analysis/nsites/impls.py	90.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[github-actions]

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
7469	6275	84%	0%	🟢

New Files

File	Coverage	Status
src/bloqade/squin/rewrite/qubit_to_stim.py	22%	🟢
src/bloqade/squin/rewrite/squin_measure.py	22%	🟢
src/bloqade/squin/rewrite/stim_rewrite_util.py	16%	🟢
src/bloqade/squin/rewrite/wire_identity_elimination.py	42%	🟢
src/bloqade/squin/rewrite/wire_to_stim.py	24%	🟢
src/bloqade/squin/rewrite/wrap_analysis.py	57%	🟢
TOTAL	30%	🟢

Modified Files

File	Coverage	Status
src/bloqade/analysis/address/impls.py	78%	🟢
src/bloqade/squin/analysis/nsites/_init_.py	100%	🟢
src/bloqade/squin/analysis/nsites/impls.py	92%	🟢
src/bloqade/squin/rewrite/_init_.py	100%	🟢
TOTAL	92%	🟢

updated for commit: dbc105c by action🐍

[johnzl-777]

Hi @Roger-luo , I believe what you see here should be a decent first pass with me trying to work with a bit of a mismatch in terms of how squin can be mapped to the existing stim dialect. Feel free to let me know what you think would be acceptable additions to either dialect based on my observations below, I can gladly spin that out as a separate PR/RFC.

I haven't taken advantage of the site analysis results just yet but it should be very easy for me to implement, I just wanted to make sure the analyses and overall rewrite behavior were (mostly) correct.

• Reset as well as MeasureAndReset in the squin.qubit dialect line up more nicely with the Stim dialect as opposed to the squin.wire dialect owing to the fact that squin.qubit can take a whole ilist of qubits while you can only take a single wire per statement in squin.wire.
• I know there's the ongoing RFC for Measurement (RFC: refactoring measurement #158 ) but I think it's also worth looking at how flexible Reset should be in both squin dialects because Stim supports Reset Z (which I default to) but you have RX and RY. Perhaps allowing for an operator from squin.op or string option to be passed in to specify this?
• Measure and MeasureAndReset both can have a result that isn't present in the Stim dialect and means I can't "eliminate" the old squin statements because the result could be depended on down the line. My understanding is you follow up your measurements with detector annotations but I assume this is something the user should specify and might also need to be added to the squin dialect?
  • EDIT: Realized @kaihsin has addressed this already with another thing that I'll work on and we already discussed, jumped the gun here 😅
• MeasureAndReset seems to have a more native representation in Stim with options for MRZ, MRY, and MRX. Currently I have to do a bit of a clunky two-statement representation: MZ, then RZ
• Should there be a way in squin to say I want the adjoint of some of the default squin.op's? I ask this because all Gate subclasses in stim.gate have this option

[Roger-luo]

I know there's the ongoing RFC for Measurement (#158 ) but I think it's also worth looking at how flexible Reset should be in both squin dialects because Stim supports Reset Z (which I default to) but you have RX and RY. Perhaps allowing for an operator from squin.op or string option to be passed in to specify this?

Yeah, can you start an RFC issue for reset? I think reset needs some more thoughts.

I think for the first pass, we can just skip those doesn't match well - squin is more expressive so not everything can be compiled into stim.

[Roger-luo]

xref #25 I realize we need a codegen for stim instead of a rewrite. And to make the UX better, we need a separate analysis pass to validate all possible incompatibility of stim when compiling from squin.

[johnzl-777]

xref #25 I realize we need a codegen for stim instead of a rewrite. And to make the UX better, we need a separate analysis pass to validate all possible incompatibility of stim when compiling from squin.

I think this partly resolves the ongoing thread between myself, @ChenZhao44 and @weinbe58 over handling the friction from the fact that Measure as well as MeasureAndReset in squin can have a result that can be depended on later down the line while stim has no such thing (that is, there is a target measurement record/detector annotation but that doesn't have an explicit "plug in directly from this measurement" and therefore the same direct linkage as squin)

I know @ChenZhao44 was advocating for just throwing an exception and killing the rewrite if it detected any form of squin measurement being depended on while @weinbe58 pointed out it would be more proper to just "let it slide" and say no rewrite happened but let emission choke on it instead.

@weinbe58 also suggested potentially "manipulating" the squin measurement result if it's being returned to something like "None" @Roger-luo has pointed out this easily violates the original intent of the function (and I'm sure causes even more problems downstream).

I support @Roger-luo 's direction here as well as the result of @kaihsin 's and @weinbe58 's conversations that codegen seems more sensible here.

[Roger-luo]

so rewrites technically are no-raise because the rewrite rules happens only when we are sure about the equivalence. If it cannot perform the rewrite, it will not be applied to the code instead of throwing an exception.

So does codegen - we want to be able to catch as many issues as we can before running any codegen or rewrite so we can have a nice error message. This is kinda true for all the possible codegen/rewrite because you rely on compile-time info to do these codegen/rewrite. I think it's always good practice to do around of validation to be certain on the entire program before mutating/generating the code that one intends to do.

[Roger-luo]

I mean we could raise within a rewrite rule, I kinda understand that is convenience to work with, but on the other hand, it will be nasty for a user because now validation is coupled with rewrite, and this would terminate rewrite process - meaning if there are multiple errors, one have to keep trying and failing until all errors are being address-ed, when all errors are technically possible to be addressed in one-go with an analysis.

[johnzl-777]

Got it 👍 I'll open up an issue to sketch out/track progress on the analysis passes to catch Stim-incompatible squin programs

[johnzl-777]

I believe this rewrite now accepts Broadcast as well as changes from #158. This will probably have to change once #265 goes through but I don't think it will be too drastic a change.

There are a couple things (ordered in what I believe is the correct priority) I think need to be done as well from my notes in talking with @ChenZhao44

• The noise statements in squin also need to map to the stim statements (I could probably knock this out in a day)
• There's also the matter of another QEC dialect that should be rewritten to stim as well that sits inbetween Chen's logical work
• There are still cases (maybe this is resolvable through another pass of sorts) where running DCE, because of potential dependencies from a lot of wire statements results, means the resulting stim rewrite from squin has dangling chunks/statements. I think some of my comments highlight this (or some of the unit tests too)
• Unit tests should of course, be made more robust (much of what is currently present is just "by eye" testing of output) but considering how much stuff is changing and being added, along with time pressure this will have to be deprioritized for now.

[johnzl-777]

The stim codegen keeps complaining about duplicate registered methods for lowering constants?
Even when I do get it to work (I add the stim subdialects instead of stim in its entirety to the dialect group) then the output comes out very silly, something like:

H constant constant constant
Z constant constant

I also realize I could definitely split out more of the rewrite logic into their own rules considering how much duplicate logic there is (with just a small variation required to handle address type extraction and wire routing). Probably worth a separate PR but in the interest of time I won't do it here (unless indicated otherwise!)

[kaihsin]

leave some comments. The question I have is which dialect that attribute should go to, but I don't think its a blocker.

[kaihsin]

is this only register to wire? or qubit dialect also need this? @Roger-luo

[johnzl-777]

This makes sense to me considering wire's Apply, Broadcast, and MeasureAndReset will return things whereas qubit doesn't. I haven't run into a case where the analysis fails on the qubit dialect.